-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[3][cleanup][gcs] Remove redis based pubsub. #23520
Conversation
…3-cpp-pubsub-remove
…3-cpp-pubsub-remove
…3-cpp-pubsub-remove
@mwtian could you help review this PR as well? |
Will take a look tomorrow. |
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 overall! Just a question about manual vs automatic edits to the mocks. There are two tests failed to build too.
src/mock/ray/pubsub/publisher.h
Outdated
@@ -84,30 +78,40 @@ class MockPublisherInterface : public PublisherInterface { | |||
namespace ray { | |||
namespace pubsub { | |||
|
|||
instrumented_io_context __io_service; |
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.
Double underscore prefixes are reserved for compilers. Ideally, we don't definite variables in headers, unless with inline
or as a static local variable: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables.
Also IIUC files under src/mock
are generated programmatically at the beginning. Should we keep it that way and put customized mocks in other locations? Then we would be able to re-run the tool to generate new mocks if needed. Otherwise it seems easy for mocks and actual interfaces to diverge.
Why are these changes needed?
This PR removes redis based pubsub.
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.