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/09 12:51:19 UTC
[GitHub] [ratis] codings-dan opened a new pull request #597: RATIS-1520. Implement step down leader in server
codings-dan opened a new pull request #597:
URL: https://github.com/apache/ratis/pull/597
## What changes were proposed in this pull request?
subtask to support no leader mode in ratis: Implement step down leader in server
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1520
## 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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #597:
URL: https://github.com/apache/ratis/pull/597#discussion_r804455942
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/StepDownLeaderRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ratis.protocol;
+
+public class StepDownLeaderRequest extends RaftClientRequest{
Review comment:
down
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #597:
URL: https://github.com/apache/ratis/pull/597#issuecomment-1035993498
@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 merged pull request #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #597:
URL: https://github.com/apache/ratis/pull/597
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #597:
URL: https://github.com/apache/ratis/pull/597#issuecomment-1038874606
@szetszwo Thanks for reviewing code, I have changed it according to the comment you left, 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] codings-dan commented on pull request #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #597:
URL: https://github.com/apache/ratis/pull/597#issuecomment-1033752081
The ci test error has nothing to do with this code change
```
Error: Tests run: 12, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 43.076 s <<< FAILURE! - in org.apache.ratis.server.simulation.TestLeaderElectionWithSimulatedRpc
Error: testEnforceLeader(org.apache.ratis.server.simulation.TestLeaderElectionWithSimulatedRpc) Time elapsed: 10.937 s <<< FAILURE!
org.junit.ComparisonFailure: expected:<s[2]> but was:<s[4]>
at org.junit.Assert.assertEquals(Assert.java:117)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.apache.ratis.server.impl.LeaderElectionTests.enforceLeader(LeaderElectionTests.java:249)
at org.apache.ratis.server.impl.LeaderElectionTests.testEnforceLeader(LeaderElectionTests.java:235)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:[498](https://github.com/apache/ratis/runs/5125324814?check_suite_focus=true#step:5:498))
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
```
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #597:
URL: https://github.com/apache/ratis/pull/597#discussion_r804540181
##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/leader/LeaderState.java
##########
@@ -30,7 +30,8 @@
public interface LeaderState {
/** The reasons that this leader steps down and becomes a follower. */
enum StepDownReason {
- HIGHER_TERM, HIGHER_PRIORITY, LOST_MAJORITY_HEARTBEATS, STATE_MACHINE_EXCEPTION, JVM_PAUSE;
+ HIGHER_TERM, HIGHER_PRIORITY, LOST_MAJORITY_HEARTBEATS, STATE_MACHINE_EXCEPTION, JVM_PAUSE,
+ NO_LEADER_MODE;
Review comment:
Let's call it "FORCE".
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -536,6 +538,9 @@ void submitStepDownEvent(long term, StepDownReason reason) {
private void stepDown(long term, StepDownReason reason) {
try {
server.changeToFollowerAndPersistMetadata(term, reason);
+ if (reason.equals(StepDownReason.NO_LEADER_MODE)) {
+ stepDownLeader.completeStepDownLeader();
+ }
Review comment:
Even if the reason is not NO_LEADER_MODE, we should complete the request, i.e.
```
private void stepDown(long term, StepDownReason reason) {
try {
server.changeToFollowerAndPersistMetadata(term, reason);
+ pendingStepDown.complete();
} catch(IOException e) {
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -547,6 +552,10 @@ private void stepDown(long term, StepDownReason reason) {
}
}
+ StepDownLeader getStepDownLeader() {
+ return stepDownLeader;
+ }
+
Review comment:
Let's add a submitStepDownRequestAsync instead.
```
CompletableFuture<RaftClientReply> submitStepDownRequestAsync(TransferLeadershipRequest request) {
return pendingStepDown.stepDownLeaderAsync(request);
}
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/StepDownLeader.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ratis.server.impl;
+
+import org.apache.ratis.protocol.RaftClientReply;
+import org.apache.ratis.protocol.TransferLeadershipRequest;
+import org.apache.ratis.protocol.exceptions.TimeoutIOException;
+import org.apache.ratis.server.leader.LeaderState;
+import org.apache.ratis.util.JavaUtils;
+import org.apache.ratis.util.MemoizedSupplier;
+import org.apache.ratis.util.TimeDuration;
+import org.apache.ratis.util.TimeoutScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+public class StepDownLeader {
+ public static final Logger LOG = LoggerFactory.getLogger(StepDownLeader.class);
+
+ class PendingRequest {
+ private final TransferLeadershipRequest request;
+ private final CompletableFuture<RaftClientReply> replyFuture = new CompletableFuture<>();
+
+ PendingRequest(TransferLeadershipRequest request) {
+ this.request = request;
+ }
+
+ TransferLeadershipRequest getRequest() {
+ return request;
+ }
+
+ CompletableFuture<RaftClientReply> getReplyFuture() {
+ return replyFuture;
+ }
+
+ void complete() {
+ LOG.info("Successfully step down leader at {} for request {}", server.getMemberId(), request);
+ replyFuture.complete(server.newSuccessReply(request));
+ }
+
+ void timeout() {
+ replyFuture.completeExceptionally(new TimeoutIOException(
+ ": Failed to step down leader on " + server.getMemberId() + "request " + request.getTimeoutMs() + "ms"));
+ }
+
+ @Override
+ public String toString() {
+ return request.toString();
+ }
+ }
+
+
+ static class PendingRequestReference {
+ private final AtomicReference<PendingRequest> ref = new AtomicReference<>();
+
+ Optional<PendingRequest> get() {
+ return Optional.ofNullable(ref.get());
+ }
+
+ Optional<PendingRequest> getAndSetNull() {
+ return Optional.ofNullable(ref.getAndSet(null));
+ }
+
+ PendingRequest getAndUpdate(Supplier<PendingRequest> supplier) {
+ return ref.getAndUpdate(p -> p != null? p: supplier.get());
+ }
+ }
+
+ private final RaftServerImpl server;
Review comment:
We should have LeaderStateImpl instead.
##########
File path: ratis-server/src/test/java/org/apache/ratis/server/impl/PreAppendLeaderStepDownTest.java
##########
@@ -108,4 +112,24 @@ private void runTestLeaderStepDown(CLUSTER cluster) throws Exception {
cluster.shutdown();
}
}
+
+ @Test
+ public void testLeaderStepDownAsync() throws Exception {
+ runWithNewCluster(3, this::runTestLeaderStepDownAsync);
+ }
+
+ void runTestLeaderStepDownAsync(CLUSTER cluster) throws IOException, InterruptedException {
Review comment:
This is an extra space before "void".
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -1059,6 +1059,25 @@ SnapshotManagementRequestHandler getSnapshotRequestHandler() {
getId() + ": Request not supported " + request));
}
+ CompletableFuture<RaftClientReply> stepDownLeaderAsync(TransferLeadershipRequest request) throws IOException {
+ LOG.info("{} receive stepDown leader request {}", getMemberId(), request);
+ assertLifeCycleState(LifeCycle.States.RUNNING);
+ assertGroup(request.getRequestorId(), request.getRaftGroupId());
+
+ CompletableFuture<RaftClientReply> reply = checkLeaderState(request, null, true);
+ if (reply != null) {
+ return CompletableFuture.completedFuture(newSuccessReply(request));
+ }
+ synchronized (this) {
+ reply = checkLeaderState(request, null, false);
+ if (reply != null) {
+ return CompletableFuture.completedFuture(newSuccessReply(request));
+ }
+ final LeaderStateImpl leaderState = role.getLeaderStateNonNull();
+ return leaderState.getStepDownLeader().stepDownLeaderAsync(request);
+ }
+ }
Review comment:
The code can be simplified as below.
```
CompletableFuture<RaftClientReply> stepDownLeaderAsync(TransferLeadershipRequest request) throws IOException {
LOG.info("{} receive stepDown leader request {}", getMemberId(), request);
assertLifeCycleState(LifeCycle.States.RUNNING);
assertGroup(request.getRequestorId(), request.getRaftGroupId());
return role.getLeaderState().map(leader -> leader.submitStepDownRequestAsync(request))
.orElseGet(() -> CompletableFuture.completedFuture(newSuccessReply(request)));
}
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/StepDownLeader.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ratis.server.impl;
+
+import org.apache.ratis.protocol.RaftClientReply;
+import org.apache.ratis.protocol.TransferLeadershipRequest;
+import org.apache.ratis.protocol.exceptions.TimeoutIOException;
+import org.apache.ratis.server.leader.LeaderState;
+import org.apache.ratis.util.JavaUtils;
+import org.apache.ratis.util.MemoizedSupplier;
+import org.apache.ratis.util.TimeDuration;
+import org.apache.ratis.util.TimeoutScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+public class StepDownLeader {
Review comment:
Let's rename it to "PendingStepDown".
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/StepDownLeader.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ratis.server.impl;
+
+import org.apache.ratis.protocol.RaftClientReply;
+import org.apache.ratis.protocol.TransferLeadershipRequest;
+import org.apache.ratis.protocol.exceptions.TimeoutIOException;
+import org.apache.ratis.server.leader.LeaderState;
+import org.apache.ratis.util.JavaUtils;
+import org.apache.ratis.util.MemoizedSupplier;
+import org.apache.ratis.util.TimeDuration;
+import org.apache.ratis.util.TimeoutScheduler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
+public class StepDownLeader {
+ public static final Logger LOG = LoggerFactory.getLogger(StepDownLeader.class);
+
+ class PendingRequest {
+ private final TransferLeadershipRequest request;
+ private final CompletableFuture<RaftClientReply> replyFuture = new CompletableFuture<>();
+
+ PendingRequest(TransferLeadershipRequest request) {
+ this.request = request;
+ }
+
+ TransferLeadershipRequest getRequest() {
+ return request;
+ }
+
+ CompletableFuture<RaftClientReply> getReplyFuture() {
+ return replyFuture;
+ }
+
+ void complete() {
+ LOG.info("Successfully step down leader at {} for request {}", server.getMemberId(), request);
+ replyFuture.complete(server.newSuccessReply(request));
+ }
+
+ void timeout() {
+ replyFuture.completeExceptionally(new TimeoutIOException(
+ ": Failed to step down leader on " + server.getMemberId() + "request " + request.getTimeoutMs() + "ms"));
+ }
+
+ @Override
+ public String toString() {
+ return request.toString();
+ }
+ }
+
+
+ static class PendingRequestReference {
+ private final AtomicReference<PendingRequest> ref = new AtomicReference<>();
+
+ Optional<PendingRequest> get() {
+ return Optional.ofNullable(ref.get());
+ }
+
+ Optional<PendingRequest> getAndSetNull() {
+ return Optional.ofNullable(ref.getAndSet(null));
+ }
+
+ PendingRequest getAndUpdate(Supplier<PendingRequest> supplier) {
+ return ref.getAndUpdate(p -> p != null? p: supplier.get());
+ }
+ }
+
+ private final RaftServerImpl server;
+ private final TimeoutScheduler scheduler = TimeoutScheduler.getInstance();
+ private final PendingRequestReference pending = new PendingRequestReference();
+
+ StepDownLeader(RaftServerImpl server) {
+ this.server = server;
+ }
+
+ CompletableFuture<RaftClientReply> stepDownLeaderAsync(TransferLeadershipRequest request) {
Review comment:
Let's simply call it "submitAsync". Also, we should check the request.
```
CompletableFuture<RaftClientReply> submitAsync(TransferLeadershipRequest request) {
Preconditions.assertNull(request.getNewLeader(), "request.getNewLeader()");
```
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #597:
URL: https://github.com/apache/ratis/pull/597#discussion_r803672609
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/StepDownLeaderRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ratis.protocol;
+
+public class StepDownLeaderRequest extends RaftClientRequest{
Review comment:
Let's reuse TransferLeadershipRequest with newLeader set to null. Then, we have let code to manage.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -182,6 +182,7 @@ public long getLastAppliedIndex() {
private final TransferLeadership transferLeadership;
private final SnapshotManagementRequestHandler snapshotRequestHandler;
+ private final StepDownLeader stepDownLeader;
Review comment:
This should be moved to LeaderStateImpl since it is a leader-only request.
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #597:
URL: https://github.com/apache/ratis/pull/597
--
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 #597: RATIS-1520. Implement step down leader in server
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #597:
URL: https://github.com/apache/ratis/pull/597#discussion_r804455787
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -182,6 +182,7 @@ public long getLastAppliedIndex() {
private final TransferLeadership transferLeadership;
private final SnapshotManagementRequestHandler snapshotRequestHandler;
+ private final StepDownLeader stepDownLeader;
Review comment:
down
--
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