Closed JinZhou5042 closed 1 month ago
I think this change needs to be thought through more carefully. Line 5200 basically says "If the worker has no tasks running, shut it down." You are changing it to mean "If the worker has tasks running, shut it down".
What's your intention here?
Uh, I found I can bring back the worker logs by manually calling m.workers_shutdown(0)
, which has the following definition:
##
# Shutdown workers connected to manager.
#
# Gives a best effort and then returns the number of workers given the shutdown order.
#
# @param self Reference to the current manager object.
# @param n The number to shutdown. 0 shutdowns all workers
def workers_shutdown(self, n=0):
return cvine.vine_workers_shutdown(self._taskvine, n)
It calls the vine_workers_shutdown
in C, which I though the parameter n
indicates the number of workers we want to shut down, but not a try to shut down empty workers.
Your change reverses the sense of the function:
Before, it would prefer to shutdown empty workers and skip over busy workers. This is the desired behavior when downsizing so that you don't lose work in progress.
Now, if prefers to shutdown busy workers and skip over empty workers. I can't think of a reason why the user would want to do that.
What are you trying to accomplish?
Whoops, you are right, I was out of my right mind.
Proposed Changes
Workers are not appropriately shut down because they never go into the if statement.
Merge Checklist
The following items must be completed before PRs can be merge. Check these off to verify you have completed all steps.
make test
Run local tests prior to pushing.make format
Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lint
Run lint on source code prior to pushing.