ascherbakoff commented on a change in pull request #481: URL: https://github.com/apache/ignite-3/pull/481#discussion_r776730152
########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/NoopTimeoutStrategy.java ########## @@ -0,0 +1,32 @@ +/* + * 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.ignite.raft.jraft.util; + +import org.apache.ignite.lang.IgniteInternalException; + +public class NoopTimeoutStrategy implements TimeoutStrategy { Review comment: No-op strategy must do nothing by definition. Instead, it throws an exception currently. ########## File path: modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java ########## @@ -64,13 +64,26 @@ import org.apache.ignite.raft.jraft.storage.snapshot.SnapshotReader; import org.apache.ignite.raft.jraft.storage.snapshot.SnapshotWriter; import org.apache.ignite.raft.jraft.util.ExecutorServiceHelper; +import org.apache.ignite.raft.jraft.util.ExponentialBackoffTimeoutStrategy; import org.apache.ignite.raft.jraft.util.JDKMarshaller; import org.jetbrains.annotations.Nullable; /** * Raft server implementation on top of forked JRaft library. */ public class JraftServerImpl implements RaftServer { + /** + * The upper bound of the election timeout adjusting. Adjusting happens according to + * {@link org.apache.ignite.raft.jraft.util.TimeoutStrategy} when a leader is not elected. + * Must be more than timeout of a membership protocol to remove failed node from the cluster. In our case, we may assume + * that 11s could be enough as far as 11s is greater than suspicion timeout for the 1000 nodes cluster with ping interval + * equals 500ms. + */ + public static final int ELECTION_TIMEOUT_MS_MAX = 11_000; Review comment: The option must be moved to the strategy. ########## File path: modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java ########## @@ -3672,4 +3719,24 @@ public void run(Status status, long theIndex, byte[] reqCtx) { latch.await(); return success.get(); } + + private void blockMessagesOnFollowers(List<Node> followers, BiPredicate<Object, String> blockingPredicate) { + for (Node follower : followers) { + RpcClientEx rpcClientEx = getRpcClientEx(follower); + rpcClientEx.blockMessages(blockingPredicate); + } + } + + private void stopBlockingMessagesOnFollowers(List<Node> followers) { + for (Node follower : followers) { + RpcClientEx rpcClientEx = getRpcClientEx(follower); + rpcClientEx.stopBlock(); + } + } + + private RpcClientEx getRpcClientEx(Node follower) { Review comment: There is already `org.apache.ignite.raft.jraft.test.TestUtils#sender` ########## File path: modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java ########## @@ -121,8 +134,12 @@ public JraftServerImpl(ClusterService service, Path dataPath, NodeOptions opts) this.opts.setSharedPools(true); if (opts.getServerName() == null) { - opts.setServerName(service.localConfiguration().getName()); + this.opts.setServerName(service.localConfiguration().getName()); } + + this.opts.setElectionTimeoutStrategy( + new ExponentialBackoffTimeoutStrategy(this.opts.getElectionTimeoutMs(), ELECTION_TIMEOUT_MS_MAX, Review comment: Use the default constructor: new ExponentialBackoffTimeoutStrategy(this.opts.getElectionTimeoutMs()) ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java ########## @@ -607,6 +612,47 @@ private void handleElectionTimeout() { } } + /** + * Method that adjusts election timeout after several consecutive unsuccessful leader elections according to {@link TimeoutStrategy} + * <p> + * Notes about general algorithm: The main idea is that in a stable cluster election timeout should be relatively small, but when + * something is preventing elections from completion, like an unstable network or long GC pauses, we don't want to have a lot of + * elections, so election timeout is adjusted. Hence, the upper bound of the election timeout adjusting is the value, which is enough to + * elect a leader or handle problems that prevent a successful leader election. + * <p> + * Leader election timeout is set to an initial value after a successful election of a leader. + */ + private void adjustElectionTimeout() { + if (options.getElectionTimeoutStrategy() instanceof NoopTimeoutStrategy) { + return; + } + + long timeout = options.getElectionTimeoutStrategy().nextTimeout(); + + if (timeout != options.getElectionTimeoutMs()) { + LOG.info("Election timeout was adjusted according to " + options.getElectionTimeoutStrategy().toString()); Review comment: Logging must use string templates. ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java ########## @@ -607,6 +612,47 @@ private void handleElectionTimeout() { } } + /** + * Method that adjusts election timeout after several consecutive unsuccessful leader elections according to {@link TimeoutStrategy} + * <p> + * Notes about general algorithm: The main idea is that in a stable cluster election timeout should be relatively small, but when + * something is preventing elections from completion, like an unstable network or long GC pauses, we don't want to have a lot of + * elections, so election timeout is adjusted. Hence, the upper bound of the election timeout adjusting is the value, which is enough to + * elect a leader or handle problems that prevent a successful leader election. + * <p> + * Leader election timeout is set to an initial value after a successful election of a leader. + */ + private void adjustElectionTimeout() { + if (options.getElectionTimeoutStrategy() instanceof NoopTimeoutStrategy) { + return; + } + + long timeout = options.getElectionTimeoutStrategy().nextTimeout(); + + if (timeout != options.getElectionTimeoutMs()) { + LOG.info("Election timeout was adjusted according to " + options.getElectionTimeoutStrategy().toString()); + resetElectionTimeoutMs((int) timeout); + electionAdjusted = true; + } + } + + /** + * Method that resets election timeout to initial value after an adjusting. + * + * For more details see {@link NodeImpl#adjustElectionTimeout()}. + */ + private void resetElectionTimeoutToInitial() { + if (!(options.getElectionTimeoutStrategy() instanceof NoopTimeoutStrategy)) { + long initialElectionTimeout = options.getElectionTimeoutStrategy().reset(); Review comment: These lines can be removed. ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java ########## @@ -607,6 +612,47 @@ private void handleElectionTimeout() { } } + /** + * Method that adjusts election timeout after several consecutive unsuccessful leader elections according to {@link TimeoutStrategy} + * <p> + * Notes about general algorithm: The main idea is that in a stable cluster election timeout should be relatively small, but when + * something is preventing elections from completion, like an unstable network or long GC pauses, we don't want to have a lot of + * elections, so election timeout is adjusted. Hence, the upper bound of the election timeout adjusting is the value, which is enough to + * elect a leader or handle problems that prevent a successful leader election. + * <p> + * Leader election timeout is set to an initial value after a successful election of a leader. + */ + private void adjustElectionTimeout() { + if (options.getElectionTimeoutStrategy() instanceof NoopTimeoutStrategy) { Review comment: This logic must be moved to the strategy. No-op strategy must do nothing. ########## File path: modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java ########## @@ -64,13 +64,26 @@ import org.apache.ignite.raft.jraft.storage.snapshot.SnapshotReader; import org.apache.ignite.raft.jraft.storage.snapshot.SnapshotWriter; import org.apache.ignite.raft.jraft.util.ExecutorServiceHelper; +import org.apache.ignite.raft.jraft.util.ExponentialBackoffTimeoutStrategy; import org.apache.ignite.raft.jraft.util.JDKMarshaller; import org.jetbrains.annotations.Nullable; /** * Raft server implementation on top of forked JRaft library. */ public class JraftServerImpl implements RaftServer { + /** + * The upper bound of the election timeout adjusting. Adjusting happens according to + * {@link org.apache.ignite.raft.jraft.util.TimeoutStrategy} when a leader is not elected. + * Must be more than timeout of a membership protocol to remove failed node from the cluster. In our case, we may assume + * that 11s could be enough as far as 11s is greater than suspicion timeout for the 1000 nodes cluster with ping interval + * equals 500ms. + */ + public static final int ELECTION_TIMEOUT_MS_MAX = 11_000; + + /** Max number of consecutive unsuccessful elections after which election timeout is adjusted. */ + public static final int MAX_ELECTION_ROUNDS_WITHOUT_ADJUSTING = 3; Review comment: The option must be moved to the strategy. ########## File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/ExponentialBackoffTimeoutStrategy.java ########## @@ -0,0 +1,106 @@ +/* + * 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.ignite.raft.jraft.util; + +import org.apache.ignite.internal.tostring.S; + +/** + * Timeout generation strategy. + * Increases startTimeout based on exponential backoff algorithm. + */ +public class ExponentialBackoffTimeoutStrategy implements TimeoutStrategy { Review comment: The strategy must not be stateful. This will make options not safe reusable between node restarts. You can avoid state by changing the contract to `nextTimeout(int electionRounds)`, where electionRound is the number of unsuccessful elections. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
