Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gcs pull resource reports #14336
Gcs pull resource reports #14336
Changes from 21 commits
dc9336a
6432664
ca2afd1
04b12a6
d69ca53
4b51207
652acb2
098e8c2
d92867c
58e2e78
21cc430
fe94f3b
35a32ed
3a084bc
7084689
b70ebe7
bc6b3bd
4d061d9
5c915e7
0938b04
8fb050f
dc6405e
c59e5d7
fa4b71c
98deada
0304fbf
c5ed4a2
6aba81a
64bacc3
cb8591d
cf97a73
532a8d6
2239d0e
bc941bc
088f36d
90ed7ec
d96c899
8750c09
61579e8
0b986c3
b66640a
ecebaef
2800139
a1e52b0
ff1f123
b3a717a
31eab0d
11d9677
d227826
68bd287
a8f9ce0
67d574d
4bb447d
5a34b41
b66b5c3
131d836
4646239
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Isn't this already posting it to the polling service? Remove TODO?
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.
It's hard to track, but this is being posted on the main thread, because the client pool posts all callbacks on the main thread: https://github.com/ray-project/ray/blob/master/src/ray/gcs/gcs_server/gcs_server.cc#L39
We should make it possible to specify the io service that each rpc is posted to, but I think that's out of scope for this PR (and will require additional synchronization work).
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.
Could we just post the entire callback to the polling_service_ and remove the TODO? I think that would also make the code simpler.
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 could, but I'm not sure it would make things simpler. This way, we only update the resource manager from the main thread, so we don't have to worry about locking it.
Btw won't posting the entire callback require deep copying the entire resource report?
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 see, is the resource manager not thread-safe?
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.
Yeah unfortunately not (in fact my understanding is that most of gcs isn't actually thread safe)