-
Notifications
You must be signed in to change notification settings - Fork 91
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
[admin-tool][controller] Add new config into ZK and allow admin-tool to update the config #1418
[admin-tool][controller] Add new config into ZK and allow admin-tool to update the config #1418
Conversation
dfade0e
to
f1479e6
Compare
...controller/src/main/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutes.java
Outdated
Show resolved
Hide resolved
...oller/src/main/java/com/linkedin/venice/controller/server/ClusterAdminOpsRequestHandler.java
Outdated
Show resolved
Hide resolved
fd77b1d
to
b5f01d7
Compare
b5f01d7
to
1bf784b
Compare
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.
Thanks a lot for the hard work, Minh! Let's create a backlog ticket to remind us to write a README for the open-source community on using this config during an admin protocol upgrade.
clients/venice-admin-tool/src/test/java/com/linkedin/venice/TestAdminTool.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/AdminTopicMetadataAccessor.java
Show resolved
Hide resolved
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
...ce-controller/src/main/java/com/linkedin/venice/controller/ZkAdminTopicMetadataAccessor.java
Outdated
Show resolved
Hide resolved
...controller/src/main/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutes.java
Show resolved
Hide resolved
...st-common/src/integrationTest/java/com/linkedin/venice/controller/TestAdminToolEndToEnd.java
Outdated
Show resolved
Hide resolved
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.
Let’s make sure to add a gRPC endpoint for the newly added API, either in this PR or a follow-up. Otherwise, we'll end up constantly playing catch-up. Thanks!
...ontroller/src/test/java/com/linkedin/venice/controller/TestZkAdminTopicMetadataAccessor.java
Outdated
Show resolved
Hide resolved
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.
Thanks for the hard work Minh!
#1535) Add grpc endpoint for updateAdminOperationProtocolVersion Input: clusterName, target admin operation version we want to set ( -1 for the latest version) Output: clusterName and updated admin operation version. This PR only contains the gRPC setup for the endpoint, the main logic can be found in PR #1418
Summary
We will introduce a new field in adminTopicMetadata in ZK at cluster-level to store the admin operation version to serialize/deserialize the message.
Why?
Please take a look at the design doc here. This PR is the first step to remove protocol upgrade deployment order. The expectation is that, with the remote protocol version at cluster-level, the deserializer will use the version, which the writer used to serialize AdminOperation, to deserialize the message. We will fail fast if necessary if the schema versions mismatch.
How was this PR tested?
added UTs and integration tests
What's next?
Update the serializer to fetch the version in ZK before serialize the message, and pass over this version for deserializer to deserialize.
Does this PR introduce any user-facing changes?