You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/02/16 02:02:57 UTC
[GitHub] [ratis] codings-dan opened a new pull request #602: RATIS-1525. Add step down leader proto and client related code
codings-dan opened a new pull request #602:
URL: https://github.com/apache/ratis/pull/602
## What changes were proposed in this pull request?
subtask to support safemode: add step down leader proto and client related code
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1525
## How was this patch tested?
UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r808618547
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -18,6 +18,7 @@
package org.apache.ratis.client.impl;
import java.nio.ByteBuffer;
+import java.util.Objects;
Review comment:
Thanks for pointing out the error, I have fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #602:
URL: https://github.com/apache/ratis/pull/602
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #602:
URL: https://github.com/apache/ratis/pull/602#issuecomment-1041469879
@szetszwo PTAL again, thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r808028770
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -519,22 +520,38 @@ static SetConfigurationRequestProto toSetConfigurationRequestProto(
static TransferLeadershipRequest toTransferLeadershipRequest(
TransferLeadershipRequestProto p) {
final RaftRpcRequestProto m = p.getRpcRequest();
- final RaftPeer newLeader = ProtoUtils.toRaftPeer(p.getNewLeader());
- return new TransferLeadershipRequest(
- ClientId.valueOf(m.getRequestorId()),
- RaftPeerId.valueOf(m.getReplyId()),
- ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
- p.getRpcRequest().getCallId(),
- newLeader.getId(),
- m.getTimeoutMs());
+ String address = p.getNewLeaderOrBuilder().getAddress();
+ if (!Objects.equals(address, "")) {
Review comment:
We should use p.hasNewLeader():
```
static TransferLeadershipRequest toTransferLeadershipRequest(
TransferLeadershipRequestProto p) {
final RaftRpcRequestProto m = p.getRpcRequest();
- final RaftPeer newLeader = ProtoUtils.toRaftPeer(p.getNewLeader());
+ final RaftPeerId newLeader = p.hasNewLeader()? ProtoUtils.toRaftPeer(p.getNewLeader()).getId(): null;
return new TransferLeadershipRequest(
ClientId.valueOf(m.getRequestorId()),
RaftPeerId.valueOf(m.getReplyId()),
ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
p.getRpcRequest().getCallId(),
- newLeader.getId(),
+ newLeader,
m.getTimeoutMs());
}
```
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -519,22 +520,38 @@ static SetConfigurationRequestProto toSetConfigurationRequestProto(
static TransferLeadershipRequest toTransferLeadershipRequest(
TransferLeadershipRequestProto p) {
final RaftRpcRequestProto m = p.getRpcRequest();
- final RaftPeer newLeader = ProtoUtils.toRaftPeer(p.getNewLeader());
- return new TransferLeadershipRequest(
- ClientId.valueOf(m.getRequestorId()),
- RaftPeerId.valueOf(m.getReplyId()),
- ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
- p.getRpcRequest().getCallId(),
- newLeader.getId(),
- m.getTimeoutMs());
+ String address = p.getNewLeaderOrBuilder().getAddress();
+ if (!Objects.equals(address, "")) {
+ return new TransferLeadershipRequest(
+ ClientId.valueOf(m.getRequestorId()),
+ RaftPeerId.valueOf(m.getReplyId()),
+ ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
+ p.getRpcRequest().getCallId(),
+ ProtoUtils.toRaftPeer(p.getNewLeader()).getId(),
+ m.getTimeoutMs());
+ } else {
+ return new TransferLeadershipRequest(
+ ClientId.valueOf(m.getRequestorId()),
+ RaftPeerId.valueOf(m.getReplyId()),
+ ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
+ p.getRpcRequest().getCallId(),
+ null,
+ m.getTimeoutMs());
+ }
}
static TransferLeadershipRequestProto toTransferLeadershipRequestProto(
TransferLeadershipRequest request) {
- return TransferLeadershipRequestProto.newBuilder()
- .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
- .setNewLeader(RaftPeer.newBuilder().setId(request.getNewLeader()).build().getRaftPeerProto())
- .build();
+ if (request.getNewLeader() != null) {
+ return TransferLeadershipRequestProto.newBuilder()
+ .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
+ .setNewLeader(RaftPeer.newBuilder().setId(request.getNewLeader()).build().getRaftPeerProto())
+ .build();
+ } else {
+ return TransferLeadershipRequestProto.newBuilder()
+ .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
+ .build();
+ }
Review comment:
Let's combine the common code:
```
static TransferLeadershipRequestProto toTransferLeadershipRequestProto(
TransferLeadershipRequest request) {
final TransferLeadershipRequestProto.Builder b = TransferLeadershipRequestProto.newBuilder()
.setRpcRequest(toRaftRpcRequestProtoBuilder(request));
Optional.ofNullable(request.getNewLeader())
.map(l -> RaftPeer.newBuilder().setId(l).build())
.map(RaftPeer::getRaftPeerProto)
.ifPresent(b::setNewLeader);
return b.build();
}
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -967,6 +967,10 @@ void finishTransferLeadership() {
transferLeadership.finish(state.getLeaderId(), false);
}
+ public RaftClientReply stepDownLeader(TransferLeadershipRequest request) throws IOException {
+ return waitForReply(request, stepDownLeaderAsync(request));
+ }
+
public CompletableFuture<RaftClientReply> transferLeadershipAsync(TransferLeadershipRequest request)
Review comment:
Let's change transferLeadershipAsync. Then, we don't have to change RaftServerProxy at all. BTW, could you also remove "public" from transferLeadership(..) and transferLeadershipAsync(..)?
```
+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
@@ -948,7 +948,7 @@ class RaftServerImpl implements RaftServer.Division,
}
}
- public RaftClientReply transferLeadership(TransferLeadershipRequest request) throws IOException {
+ RaftClientReply transferLeadership(TransferLeadershipRequest request) throws IOException {
return waitForReply(request, transferLeadershipAsync(request));
}
@@ -967,8 +967,12 @@ class RaftServerImpl implements RaftServer.Division,
transferLeadership.finish(state.getLeaderId(), false);
}
- public CompletableFuture<RaftClientReply> transferLeadershipAsync(TransferLeadershipRequest request)
+ CompletableFuture<RaftClientReply> transferLeadershipAsync(TransferLeadershipRequest request)
throws IOException {
+ if (request.getNewLeader() == null) {
+ return stepDownLeaderAsync(request);
+ }
+
LOG.info("{}: receive transferLeadership {}", getMemberId(), request);
assertLifeCycleState(LifeCycle.States.RUNNING);
assertGroup(request.getRequestorId(), request.getRaftGroupId());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r808119398
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -519,22 +520,38 @@ static SetConfigurationRequestProto toSetConfigurationRequestProto(
static TransferLeadershipRequest toTransferLeadershipRequest(
TransferLeadershipRequestProto p) {
final RaftRpcRequestProto m = p.getRpcRequest();
- final RaftPeer newLeader = ProtoUtils.toRaftPeer(p.getNewLeader());
- return new TransferLeadershipRequest(
- ClientId.valueOf(m.getRequestorId()),
- RaftPeerId.valueOf(m.getReplyId()),
- ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
- p.getRpcRequest().getCallId(),
- newLeader.getId(),
- m.getTimeoutMs());
+ String address = p.getNewLeaderOrBuilder().getAddress();
+ if (!Objects.equals(address, "")) {
Review comment:
done
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/ClientProtoUtils.java
##########
@@ -519,22 +520,38 @@ static SetConfigurationRequestProto toSetConfigurationRequestProto(
static TransferLeadershipRequest toTransferLeadershipRequest(
TransferLeadershipRequestProto p) {
final RaftRpcRequestProto m = p.getRpcRequest();
- final RaftPeer newLeader = ProtoUtils.toRaftPeer(p.getNewLeader());
- return new TransferLeadershipRequest(
- ClientId.valueOf(m.getRequestorId()),
- RaftPeerId.valueOf(m.getReplyId()),
- ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
- p.getRpcRequest().getCallId(),
- newLeader.getId(),
- m.getTimeoutMs());
+ String address = p.getNewLeaderOrBuilder().getAddress();
+ if (!Objects.equals(address, "")) {
+ return new TransferLeadershipRequest(
+ ClientId.valueOf(m.getRequestorId()),
+ RaftPeerId.valueOf(m.getReplyId()),
+ ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
+ p.getRpcRequest().getCallId(),
+ ProtoUtils.toRaftPeer(p.getNewLeader()).getId(),
+ m.getTimeoutMs());
+ } else {
+ return new TransferLeadershipRequest(
+ ClientId.valueOf(m.getRequestorId()),
+ RaftPeerId.valueOf(m.getReplyId()),
+ ProtoUtils.toRaftGroupId(m.getRaftGroupId()),
+ p.getRpcRequest().getCallId(),
+ null,
+ m.getTimeoutMs());
+ }
}
static TransferLeadershipRequestProto toTransferLeadershipRequestProto(
TransferLeadershipRequest request) {
- return TransferLeadershipRequestProto.newBuilder()
- .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
- .setNewLeader(RaftPeer.newBuilder().setId(request.getNewLeader()).build().getRaftPeerProto())
- .build();
+ if (request.getNewLeader() != null) {
+ return TransferLeadershipRequestProto.newBuilder()
+ .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
+ .setNewLeader(RaftPeer.newBuilder().setId(request.getNewLeader()).build().getRaftPeerProto())
+ .build();
+ } else {
+ return TransferLeadershipRequestProto.newBuilder()
+ .setRpcRequest(toRaftRpcRequestProtoBuilder(request))
+ .build();
+ }
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r807574834
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
##########
@@ -40,4 +40,7 @@ default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf) throws IOE
/** Transfer leadership to the given server.*/
RaftClientReply transferLeadership(RaftPeerId newLeader, long timeoutMs) throws IOException;
+
+ /** Step down the leader.*/
+ RaftClientReply stepDownLeader(RaftPeerId newLeader, long timeoutMs) throws IOException;
Review comment:
Let's reuse transferLeadership with newLeader==null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r808119035
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -967,6 +967,10 @@ void finishTransferLeadership() {
transferLeadership.finish(state.getLeaderId(), false);
}
+ public RaftClientReply stepDownLeader(TransferLeadershipRequest request) throws IOException {
+ return waitForReply(request, stepDownLeaderAsync(request));
+ }
+
public CompletableFuture<RaftClientReply> transferLeadershipAsync(TransferLeadershipRequest request)
Review comment:
Really should restrict permissions and delete public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #602:
URL: https://github.com/apache/ratis/pull/602#issuecomment-1041561733
@codings-dan , could you also remove `RaftServerImpl.setLeaderElectionPause`. It is never used.
```
@@ -1448,11 +1452,6 @@ class RaftServerImpl implements RaftServer.Division,
return reply;
}
- void setLeaderElectionPause(boolean pause) throws ServerNotReadyException {
- assertLifeCycleState(LifeCycle.States.RUNNING);
- role.setLeaderElectionPause(pause);
- }
-
boolean pause() {
// TODO: should pause() be limited on only working for a follower?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on a change in pull request #602: RATIS-1525. Add step down leader proto and client related code
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #602:
URL: https://github.com/apache/ratis/pull/602#discussion_r807934927
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
##########
@@ -40,4 +40,7 @@ default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf) throws IOE
/** Transfer leadership to the given server.*/
RaftClientReply transferLeadership(RaftPeerId newLeader, long timeoutMs) throws IOException;
+
+ /** Step down the leader.*/
+ RaftClientReply stepDownLeader(RaftPeerId newLeader, long timeoutMs) throws IOException;
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org