[GitHub] [buildstream] abderrahim commented on pull request #1706: simplify jobs

[GitBox <gi...@apache.org>]

abderrahim commented on PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#issuecomment-1203597928

   >     * I'd like to keep the separation of `Job` and `ElementJob` for reasons stated above
   
   The main reason I decided to merge `ElementJob` into job is the fact that it didn't do much: it just replaced the abstract method to override with a callback function, and everything else was using the callbacks instead of subclassing.
   
   It is true that there is an `Element` specific part in it, namely the fact that an `Element` is passed to the constructor (and subsequently to the callback functions). But the `Element`-specific logging stuff is already in the parent `Job` class (exposed to subclasses via protected methods).
   
   Would you be open to reworking this patch such that the `element` passed to the job can be `None` (and even have `None` default value), and document how to make a `Job` without an `Element`?
   
   >     * I agree that the simplification of `job.py` and elimination of the separate `ChildJob` object is desirable, as I believe the first patch in this branch achieves
   
   Ack.
   
   >       * I'm a little concerned about this patch, just because it appears to touch on task termination logic. BuildStream suspension and termination, especially in interactive scenarios, is really not very well covered in the test suite, so I would just appreciate that some creative interactive testing be done to avoid regressions on this front.
   
   It doesn't. The only change it does is a cut-and-paste of `ChildJob.terminate()` into where it was called in `Job.terminate()`, and this is not a big function or anything.
   
   And to be honest, this was my main motivation for touching this part of the code. I'd like to try to fix #1613, and having this back and forth between two classes isn't helping me understand.
   
   I appreciate that this is a delicate problem (as we already have #1613 and another instance where we get a BUG message when terminating), but I think this PR isn't touching that and shouldn't be blocked by testing.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org