sashapolo commented on code in PR #4129:
URL: https://github.com/apache/ignite-3/pull/4129#discussion_r1691166237
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/CliRequests.java:
##########
@@ -89,7 +99,8 @@ public interface ChangePeersAsyncRequest extends Message {
Collection<String> newLearnersList();
- long term();
+ // term is intentionally Long and not long in order to perform
nullable (not initialized) check.
+ Long term();
Review Comment:
Same question
##########
modules/raft/src/test/java/org/apache/ignite/raft/jraft/rpc/impl/cli/ChangePeersRequestProcessorTest.java:
##########
@@ -32,13 +32,15 @@
import static org.mockito.ArgumentMatchers.eq;
public class ChangePeersRequestProcessorTest extends
AbstractCliRequestProcessorTest<ChangePeersRequest> {
-
+ private static final long CURRENT_TERM = 1L;
@Override
Review Comment:
missing empty line
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3200,28 +3202,52 @@ public void testChangePeersAddMultiNodes() throws
Exception {
TestPeer peer = peers.get(1);
// fail, because the peers are not started.
SynchronizedClosure done = new SynchronizedClosure();
- leader.changePeers(new
Configuration(Collections.singletonList(peer.getPeerId())), done);
+ leader.changePeers(new
Configuration(Collections.singletonList(peer.getPeerId())),
leader.getCurrentTerm(), done);
assertEquals(RaftError.ECATCHUP, done.await().getRaftError());
// start peer1
assertTrue(cluster.start(peer));
// still fail, because peer2 is not started
done.reset();
- leader.changePeers(conf, done);
+ leader.changePeers(conf, leader.getCurrentTerm(), done);
assertEquals(RaftError.ECATCHUP, done.await().getRaftError());
// start peer2
peer = peers.get(2);
assertTrue(cluster.start(peer));
done.reset();
+
+ for (NodeImpl node: cluster.getNodes())
+ assertEquals(node.getConf().getConf().getPeers().size(), 1);
+
// works
- leader.changePeers(conf, done);
+ leader.changePeers(conf, leader.getCurrentTerm(), done);
Status await = done.await();
assertTrue(await.isOk(), await.getErrorMsg());
cluster.ensureSame();
assertEquals(3, cluster.getFsms().size());
+
for (MockStateMachine fsm : cluster.getFsms())
assertEquals(10, fsm.getLogs().size());
+
+ for (NodeImpl node: cluster.getNodes())
+ assertEquals(node.getConf().getConf().getPeers().size(), 3);
+
+ // another attempt to change peers with unmatched term
Review Comment:
Shall we have a separate test for the new logic? It's strange that we check
everything in one place
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/CliRequests.java:
##########
@@ -69,6 +69,11 @@ public interface ChangePeersRequest extends Message {
String leaderId();
Collection<String> newPeersList();
+
+ Collection<String> newLearnersList();
+
+ // term is intentionally Long and not long in order to perform
nullable (not initialized) check.
+ Long term();
Review Comment:
Why isn't this field marked as `@Nullable`?
--
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]
