[GitHub] [buildstream] juergbi commented on a diff in pull request #1706: simplify jobs

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

juergbi commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r930627135


##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
 # process. It has some methods that are not implemented - they are meant for
 # you to implement in a subclass.
 #
-# It has a close relationship with the ChildJob class, and it can be considered
-# a two part solution:
-#
-# 1. A Job instance, which will create a ChildJob instance and arrange for
-#    childjob.child_process() to be executed in another process.
-# 2. The created ChildJob instance, which does the actual work.
-#
-# This split makes it clear what data is passed to the other process and what
-# is executed in which process.
-#
-# To set up a minimal new kind of Job, e.g. YourJob:
-#
-# 1. Create a YourJob class, inheriting from Job.
-# 2. Create a YourChildJob class, inheriting from ChildJob.
-# 3. Implement YourJob.create_child_job() and YourJob.parent_complete().
-# 4. Implement YourChildJob.child_process().
-#
 # Args:
 #    scheduler (Scheduler): The scheduler
 #    action_name (str): The queue action name
 #    logfile (str): A template string that points to the logfile
 #                   that should be used - should contain {pid}.
+#    element (Element): The element to work on
+#    action_cb (callable): The function to execute on the child
+#    complete_cb (callable): The function to execute when the job completes
 #    max_retries (int): The maximum number of retries
 #
+# Here is the calling signature of the action_cb:
+#
+#     action_cb():
+#
+#     This function will be called in the child task
+#
+#     Args:
+#        element (Element): The element passed to the Job() constructor
+#
+#     Returns:
+#        (object): Any abstract simple python object, including a string, int,
+#                  bool, list or dict, this must be a simple serializable object.
+#
+# Here is the calling signature of the complete_cb:
+#
+#     complete_cb():
+#
+#     This function will be called when the child task completes

Review Comment:
   ```suggestion
   #     This function will be called in the main thread when the child task completes
   ```



##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
 # process. It has some methods that are not implemented - they are meant for
 # you to implement in a subclass.

Review Comment:
   There are no subclasses anymore.



##########
src/buildstream/_scheduler/job.py:
##########
@@ -27,12 +27,12 @@
 import traceback
 
 # BuildStream toplevel imports
-from ... import utils
-from ..._utils import terminate_thread
-from ..._exceptions import ImplError, BstError, set_last_task_error, SkipJob
-from ..._message import Message, MessageType
-from ...types import FastEnum
-from ..._signals import TerminateException
+from .. import utils
+from .._utils import terminate_thread
+from .._exceptions import BstError, set_last_task_error, SkipJob
+from .._message import Message, MessageType
+from ..types import FastEnum
+from .._signals import TerminateException
 
 
 # Return code values shutdown of job handling child processes

Review Comment:
   There are still a few lines mentioning processes instead of threads. Let's fix those while we're at it.



##########
src/buildstream/_scheduler/job.py:
##########
@@ -322,111 +271,23 @@ async def _parent_child_completed(self, returncode):
         else:
             status = JobStatus.FAIL
 
-        self.parent_complete(status, self._result)
+        self._complete_cb(self, self._element, status, self._result)

Review Comment:
   I'd suggest renaming the `_parent_child_completed()` method as well. Especially the 'parent' term no longer makes sense. Also the doc comments should be updated to refer to threads, not processes.



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