[Notes] [Git][BuildStream/buildstream][tristan/refactor-queues-update-state] _scheduler/queues: Don't call update state outside of error handling harness

[Tristan Van Berkom <gitlab mg gitlab com>]

Title: GitLab

Tristan Van Berkom pushed to branch tristan/refactor-queues-update-state at BuildStream / buildstream

Commits:

• 98c15463
  by Tristan Van Berkom at 2018-12-05T07:04:56Z

  _scheduler/queues: Don't call update state outside of error handling harness
  
  Commit 3fa79d8da, part of an initiative for caching of the failed builds,
  introduced a call to Element._update_state() after a job completes and before
  entering the error handling harness intended for handling plugin raised errors.
  
  Element._update_state() can result in triggering plugin code to
  run, so this is incorrect, and causes raised errors to crash BuildStream
  if they happen here.
  
  After analyzing the code, it appears that this additional call to
  Element._update_state() is unneeded, and was only added because the
  state needs to be updated for a failure as well as a success.
  
  Instead, we now have the BuildQueue call Element._assemble_done()
  unconditionally, regardless of whether the build was successful
  or not, which has the same effect and also reads better.
  
  In addition, added a FIXME comment that we are still conditionally
  updating the artifact cache size from BuildQueue.done() only
  if the build is successful, which is incorrect because failed builds
  also increase the local artifact cache size - to fix this we need
  to communicate the added artifact size through Element._assemble()
  regardless of whether the build succeeded or failed.
  

2 changed files:

• buildstream/_scheduler/queues/buildqueue.py
• buildstream/_scheduler/queues/queue.py

Changes:

buildstream/_scheduler/queues/buildqueue.py

---

...	...	@@ -106,10 +106,16 @@ class BuildQueue(Queue):
106	106
107	107	def done(self, job, element, result, success):
108	108
109		- if success:
110		- # Inform element in main process that assembly is done
111		- element._assemble_done()
	109	+ # Inform element in main process that assembly is done
	110	+ element._assemble_done()
112	111
113		- # This has to be done after _assemble_done, such that the
114		- # element may register its cache key as required
	112	+ # This has to be done after _assemble_done, such that the
	113	+ # element may register its cache key as required
	114	+ #
	115	+ # FIXME: Element._assemble() does not report both the failure state and the
	116	+ # size of the newly cached failed artifact, so we can only adjust the
	117	+ # artifact cache size for a successful build even though we know a
	118	+ # failed build also grows the artifact cache size.
	119	+ #
	120	+ if success:
115	121	self._check_cache_size(job, element, result)

buildstream/_scheduler/queues/queue.py

---

...	...	@@ -292,7 +292,6 @@ class Queue():
292	292	# See the Job object for an explanation of the call signature
293	293	#
294	294	def _job_done(self, job, element, success, result):
295		- element._update_state()
296	295
297	296	# Update values that need to be synchronized in the main task
298	297	# before calling any queue implementation

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.