Skip to content
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

[ID Refactor] Shorten the length of JobID to 4 bytes #5110

Merged
merged 43 commits into from
Jul 11, 2019

Conversation

jovany-wang
Copy link
Contributor

What do these changes do?

Some points in this PR:

(1) Shorten the length of JobID to 4 bytes.
(2) The job_id is generated by GCS for uniqueness.
(3) Embed job_id to driver id. The first 4 bytes of a driver id is the job_id bits, and rest 16bits must be filled with 0xFF.

Refer this doc for more details of ID Refactor.

Related issue number

Linter

  • I've run scripts/format.sh to lint the changes in this PR.

@jovany-wang
Copy link
Contributor Author

@zhijunfu cc

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15072/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15094/
Test FAILed.

@jovany-wang
Copy link
Contributor Author

@robertnishihara Does this commit make sense to you ?

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15106/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15108/
Test PASSed.

@robertnishihara
Copy link
Collaborator

@jovany-wang why introduce worker.current_exported_id? Isn't that the same thing as the current job ID? Or maybe the IDs are different, but there is a one-to-one correspondence between them?

@jovany-wang
Copy link
Contributor Author

jovany-wang commented Jul 5, 2019

@jovany-wang why introduce worker.current_exported_id? Isn't that the same thing as the current job ID? Or maybe the IDs are different, but there is a one-to-one correspondence between them?

@robertnishihara
Because this PR makes JobID incremented from 1 in a cluster instead of random one. that means we might get the same id in 2 sessions:

ray.init()  # job_id is 0x00000001
ray.shutdown()
ray.init()  # job_id is 0x00000001
ray.shutdown()

So, it's not appropriate to use job_id to check whether we have exported the classes and funcs in different sessions.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15112/
Test FAILed.

src/ray/common/id.h Outdated Show resolved Hide resolved
src/ray/common/id.cc Outdated Show resolved Hide resolved
src/ray/common/id.cc Outdated Show resolved Hide resolved
src/ray/common/constants.h Outdated Show resolved Hide resolved
python/ray/worker.py Outdated Show resolved Hide resolved
python/ray/worker.py Show resolved Hide resolved
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1544/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15236/
Test FAILed.

@pcmoritz pcmoritz self-assigned this Jul 9, 2019
Copy link
Contributor

@pcmoritz pcmoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, I left a few comments!

python/ray/worker.py Outdated Show resolved Hide resolved
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1576/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15266/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15270/
Test PASSed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1580/
Test FAILed.

@jovany-wang
Copy link
Contributor Author

@pcmoritz Do the responses make sense to you ?

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1600/
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/1603/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15290/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/15294/
Test PASSed.

@jovany-wang
Copy link
Contributor Author

CI failures seems not related.
@pcmoritz Let's merge this ?

@jovany-wang jovany-wang dismissed pcmoritz’s stale review July 11, 2019 06:24

All comments were addressed. Let's merge to unblock others.

@jovany-wang jovany-wang merged commit f229324 into ray-project:master Jul 11, 2019
@jovany-wang jovany-wang deleted the short-jobid-to-4bytes branch July 11, 2019 06:30
@jovany-wang jovany-wang mentioned this pull request Jul 26, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants