-
Notifications
You must be signed in to change notification settings - Fork 318
feat: add promote member support. #1500
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
feat: add promote member support. #1500
Conversation
304e69b to
97bb7fb
Compare
|
/approve |
|
@mahanteshwar-git: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @lburgazzoli , @fanminshi , @vorburger , Please review the pull request |
334d483 to
719d434
Compare
|
fixed spotless errors. |
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 adds support for promoting etcd cluster members from learner (non-voting) status to full voting members through a new promoteMember API method.
- Implements the
MemberPromotegRPC service endpoint with request/response message types - Adds
promoteMembermethod to theClusterinterface andClusterImplimplementation - Creates
MemberPromoteResponseclass to handle the promote operation response
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jetcd-grpc/src/main/proto/rpc.proto | Adds MemberPromote gRPC service method and message definitions |
| jetcd-core/src/main/proto/rpc.proto | Duplicates the gRPC service method and message definitions |
| jetcd-core/src/main/java/io/etcd/jetcd/Cluster.java | Adds promoteMember method signature to Cluster interface |
| jetcd-core/src/main/java/io/etcd/jetcd/impl/ClusterImpl.java | Implements promoteMember method in ClusterImpl |
| jetcd-core/src/main/java/io/etcd/jetcd/cluster/MemberPromoteResponse.java | Creates response wrapper class for member promote operations |
| jetcd-core/src/test/java/io/etcd/jetcd/impl/ClusterMembersTest.java | Adds integration test for the new promoteMember functionality |
Comments suppressed due to low confidence (1)
jetcd-core/src/test/java/io/etcd/jetcd/impl/ClusterMembersTest.java:131
- The test only verifies the failure case when promoting a learner that is not in sync with the leader. Consider adding a positive test case that successfully promotes a member when conditions are met, or at least verify the specific error message more precisely to ensure the correct API endpoint is being called.
// Now attempt to promote a member. Although it fails, it confirms that API was executed.
jetcd-core/src/main/java/io/etcd/jetcd/cluster/MemberPromoteResponse.java
Outdated
Show resolved
Hide resolved
jetcd-core/src/test/java/io/etcd/jetcd/impl/ClusterMembersTest.java
Outdated
Show resolved
Hide resolved
0fce633 to
79c7156
Compare
Signed-off-by: mchimang <[email protected]>
79c7156 to
db96465
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lburgazzoli, mahanteshwar-git The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @vorburger, Would it be possible to take another look at the Pr and approve if the changes look reasonable ? |
What this PR does
This PR adds support for promote member invocation for the new members which are added a learner nodes.
Why this is useful
The existing add members support adding member as a learner. However there is no support for promoting the
member as regular raft voting member. This feature attempts to address this.
Implementation Details
ClusterMembersTestto include unit tests for invoking promoteMemberRelated Issues / Discussions
This change was based on discussion in #1499
Checklist