gtristan commented on a change in pull request #1433:
URL: https://github.com/apache/buildstream/pull/1433#discussion_r562395832
##########
File path: src/buildstream/_stream.py
##########
@@ -1513,6 +1513,24 @@ def _resolve_elements(self, targets):
if task:
task.add_current_progress()
+ # _reset()
+ #
+ # Resets the internal state related to a given scheduler run.
+ #
+ # Invocations to the scheduler should start with a _reset() and end
+ # with _run() like so:
+ #
+ # self._reset()
+ # self._add_queue(...)
+ # self._add_queue(...)
+ # self._enqueue_plan(...)
+ # self._run()
Review comment:
This one I think I'll leave this way at the moment, as there isn't
really any "higher up" location for this.
The alternatives I can see would be:
* Move the general comment to `_run()`, doesn't really change much
* Setup a big commented "block" section for this portion of the stream's
internal API, I don't feel the code would benefit from further artificial
sectioning (when files get big, we usually section by public/private/internal
portions already).
To make this clearer, it would probably be best to add a higher level API to
the `Scheduler` (or perhaps a `Scheduler` abstraction API depending on what is
found to be more appropriate), and remove some visibility of the `Scheduler`
API that might not be needed by `Stream`, i.e. we could migrate this
`run()`/`reset()`/`add_queue()`/`enqueue_plan()` directly to the `Scheduler` -
this would require some additional thinking and I think is out of scope for
this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
