-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[core] Move inner_publisher logic into gcsPublisher #53905
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
base: master
Are you sure you want to change the base?
[core] Move inner_publisher logic into gcsPublisher #53905
Conversation
febab2e
to
604ecf7
Compare
Signed-off-by: You-Cheng Lin <mses010108@gmail.com>
@kevin85421 PTAL |
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.
Pull Request Overview
This PR centralizes the inner pubsub::Publisher
setup by moving its creation logic into the GcsPublisher
class.
- Added a new
GcsPublisher
constructor that accepts aPeriodicalRunner
. - Moved the channel-list initialization and related parameters into
GcsPublisher
. - Simplified
GcsServer
to use the new constructor directly.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/ray/gcs/pubsub/gcs_pub_sub.h | Added a new explicit constructor for GcsPublisher . |
src/ray/gcs/pubsub/gcs_pub_sub.cc | Implemented the new constructor with inline channel list and timing logic. |
src/ray/gcs/gcs_server/gcs_server.cc | Removed manual Publisher setup and updated to use the new GcsPublisher constructor. |
Comments suppressed due to low confidence (2)
src/ray/gcs/pubsub/gcs_pub_sub.h:51
- [nitpick] Add a doc comment above this constructor to explain its purpose, parameters, and how it differs from the existing constructor.
explicit GcsPublisher(std::shared_ptr<PeriodicalRunner> periodical_runner);
src/ray/gcs/pubsub/gcs_pub_sub.cc:27
- Consider adding unit tests for this new constructor path to verify that the publisher is initialized correctly with the provided
PeriodicalRunner
.
GcsPublisher::GcsPublisher(std::shared_ptr<PeriodicalRunner> periodical_runner)
@dayshah Would you mind also take a look of this? |
@@ -24,6 +24,23 @@ | |||
namespace ray { | |||
namespace gcs { | |||
|
|||
GcsPublisher::GcsPublisher(std::shared_ptr<PeriodicalRunner> periodical_runner) | |||
: publisher_(std::make_unique<pubsub::Publisher>( |
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.
does publisher_ need to be a unique ptr for any reason?
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 think so, cause gcs_pub_sub.h it is defined as unique ptr.
ray/src/ray/gcs/pubsub/gcs_pub_sub.h
Lines 46 to 49 in 270d340
explicit GcsPublisher(std::unique_ptr<pubsub::Publisher> publisher) | |
: publisher_(std::move(publisher)) { | |
RAY_CHECK(publisher_); | |
} |
ray/src/ray/gcs/pubsub/gcs_pub_sub.h
Lines 96 to 97 in 270d340
private: | |
const std::unique_ptr<pubsub::Publisher> publisher_; |
gonna hold off on this one for a week, I'm gonna take another look at the publishers this week / next week. I think we might decide to do a bigger cleanup soon as part of pubsub fault tolerance work. |
Why are these changes needed?
Saw the TODO comment from yic to move Inner publicher in to GCS publisher.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.