-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit maximum starting workers per language #3852
Limit maximum starting workers per language #3852
Conversation
Test PASSed. |
Test PASSed. |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left a couple comments.
/// The number of workers per process. | ||
int num_workers_per_process_; | ||
|
||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are we making all of the below protected
instead of private
? For the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the code and move some fields back to private. But I still need states_by_lang_ to be protected for tests.
src/ray/raylet/worker_pool_test.cc
Outdated
} | ||
// Check that number of starting worker processes equals to | ||
// maximum_startup_concurrency_ * 2. (because we started both python and java workers) | ||
ASSERT_EQ(worker_pool_.NumWorkerProcessesStarting(), 5 * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include a comment saying where the value 5
is initialized otherwise it is a bit out of the blue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The linting Travis build seems to be failing https://travis-ci.com/ray-project/ray/jobs/173188614 |
750ad1a
to
e21740c
Compare
Test PASSed. |
Test FAILed. |
ef5dec0
to
a3a8013
Compare
Test PASSed. |
Test PASSed. |
Test PASSed. |
What do these changes do?
Assumes that a node has 4 CPUs and we enabled both java and python workers. Before this change, raylet will start 4 java workers and 0 python workers by default at startup. After this change, raylet will start 4 java workers and 4 python workers by default at startup.
Related issue number