Skip to content

[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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

owenowenisme
Copy link
Contributor

@owenowenisme owenowenisme commented Jun 18, 2025

Why are these changes needed?

Saw the TODO comment from yic to move Inner publicher in to GCS publisher.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@owenowenisme owenowenisme changed the title moved Move inner_publisher logic into gcsPublisher Jun 18, 2025
Signed-off-by: You-Cheng Lin <mses010108@gmail.com>
@owenowenisme owenowenisme force-pushed the move-inner-publisher-into-gcs-publisher branch from febab2e to 604ecf7 Compare June 18, 2025 08:04
Signed-off-by: You-Cheng Lin <mses010108@gmail.com>
@owenowenisme owenowenisme marked this pull request as ready for review June 18, 2025 11:55
@Copilot Copilot AI review requested due to automatic review settings June 18, 2025 11:55
@owenowenisme owenowenisme requested a review from a team as a code owner June 18, 2025 11:55
@owenowenisme owenowenisme changed the title Move inner_publisher logic into gcsPublisher [core] Move inner_publisher logic into gcsPublisher Jun 18, 2025
@owenowenisme
Copy link
Contributor Author

@kevin85421 PTAL

Copy link
Contributor

@Copilot Copilot AI left a 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 a PeriodicalRunner.
  • 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)

@owenowenisme
Copy link
Contributor Author

@dayshah Would you mind also take a look of this?

@dayshah dayshah added the go add ONLY when ready to merge, run all tests label Jul 8, 2025
@@ -24,6 +24,23 @@
namespace ray {
namespace gcs {

GcsPublisher::GcsPublisher(std::shared_ptr<PeriodicalRunner> periodical_runner)
: publisher_(std::make_unique<pubsub::Publisher>(
Copy link
Contributor

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?

Copy link
Contributor Author

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.

explicit GcsPublisher(std::unique_ptr<pubsub::Publisher> publisher)
: publisher_(std::move(publisher)) {
RAY_CHECK(publisher_);
}

private:
const std::unique_ptr<pubsub::Publisher> publisher_;

@jjyao jjyao requested a review from dayshah July 14, 2025 16:08
@dayshah
Copy link
Contributor

dayshah commented Jul 17, 2025

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.

@ray-gardener ray-gardener bot added community-contribution Contributed by the community core Issues that should be addressed in Ray Core labels Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants