sanpwc commented on code in PR #4978:
URL: https://github.com/apache/ignite-3/pull/4978#discussion_r1899492961
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
Review Comment:
I guess that in case of single-node raft group both term and index of
initial configuration are the same, is that correct? If it's true, why not to
assert precise values?
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
+ index.set(-1);
+
+ sendTestTaskAndWait(leader);
+
+ TestPeer newPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 1);
+ assertTrue(cluster.start(newPeer, false, 300));
+
+ verify(raftGrpEvtsLsnr, never()).onNewPeersConfigurationApplied(any(),
any(), anyLong(), anyLong());
+
+ // Wait until new node node sees every other node, otherwise
+ // changePeersAndLearnersAsync can fail.
+ waitForTopologyOnEveryNode(1, cluster);
+
+ leader = cluster.getLeader();
Review Comment:
Why? You've already retrieved leader on line 3475. There's no way for leader
to change.
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
+ index.set(-1);
+
+ sendTestTaskAndWait(leader);
Review Comment:
Why?
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
+ index.set(-1);
+
+ sendTestTaskAndWait(leader);
+
+ TestPeer newPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 1);
+ assertTrue(cluster.start(newPeer, false, 300));
+
+ verify(raftGrpEvtsLsnr, never()).onNewPeersConfigurationApplied(any(),
any(), anyLong(), anyLong());
+
+ // Wait until new node node sees every other node, otherwise
+ // changePeersAndLearnersAsync can fail.
+ waitForTopologyOnEveryNode(1, cluster);
+
+ leader = cluster.getLeader();
+ assertNotNull(leader);
+ assertEquals(peer0.getPeerId(), leader.getNodeId().getPeerId());
+
+ SynchronizedClosure done = new SynchronizedClosure();
+ leader.changePeersAndLearnersAsync(new
Configuration(List.of(peer0.getPeerId(), newPeer.getPeerId()), List.of()),
leader.getCurrentTerm(), done);
+
+ assertEquals(done.await(), Status.OK());
+
+ assertTrue(waitForCondition(() -> {
+ if (cluster.getLeader() != null) {
Review Comment:
Why?
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
Review Comment:
The test is messy. Please check my comments below.
Basically you should check
1. That onRawConfigurationCommitted is called with proper **precise** values
on term and index on initial node start.
2. That onRawConfigurationCommitted is called with proper **precise** values
on term and index after reconfiguration.
3. That onNewPeersConfigurationApplied is called with corresponding
**precise** values.
4. That onNewPeersConfigurationApplied is not called after resetPeers.
Because currently we do not expect it to be called.
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
Review Comment:
Why?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -493,7 +493,9 @@ void reset(final Status st) {
if (listener != null) {
if (status.isOk()) {
-
listener.onNewPeersConfigurationApplied(resultPeerIds, resultLearnerIds);
+ LogId id = node.conf.getId();
Review Comment:
Did you check that node.conf.getId() matches currently processing
configuration change? Meaning that it's not the old one.
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
+ index.set(-1);
+
+ sendTestTaskAndWait(leader);
+
+ TestPeer newPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 1);
+ assertTrue(cluster.start(newPeer, false, 300));
+
+ verify(raftGrpEvtsLsnr, never()).onNewPeersConfigurationApplied(any(),
any(), anyLong(), anyLong());
+
+ // Wait until new node node sees every other node, otherwise
+ // changePeersAndLearnersAsync can fail.
+ waitForTopologyOnEveryNode(1, cluster);
+
+ leader = cluster.getLeader();
+ assertNotNull(leader);
+ assertEquals(peer0.getPeerId(), leader.getNodeId().getPeerId());
Review Comment:
Why?
##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3433,10 +3436,83 @@ public void testNewPeersConfigurationAppliedListener()
throws Exception {
return false;
}, 10_000));
- verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(List.of(newPeer), List.of(newLearner));
+ verify(raftGrpEvtsLsnr,
times(1)).onNewPeersConfigurationApplied(eq(List.of(newPeer)),
eq(List.of(newLearner)), anyLong(), anyLong());
}
}
+ @Test
+ public void
testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied() throws
Exception {
+ TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT);
+ var raftGrpEvtsLsnr = mock(JraftGroupEventsListener.class);
+
+ AtomicLong term = new AtomicLong(-1);
+ AtomicLong index = new AtomicLong(-1);
+
+ cluster = new TestCluster(
+
"testIndexAndTermArePropagatedToOnNewPeersConfigurationApplied",
+ dataPath,
+ Collections.singletonList(peer0),
+ new LinkedHashSet<>(),
+ ELECTION_TIMEOUT_MILLIS,
+ (peerId, opts) -> {
+ opts.setRaftGrpEvtsLsnr(raftGrpEvtsLsnr);
+ opts.setFsm(new MockStateMachine(peerId) {
+ @Override
+ public void
onRawConfigurationCommitted(ConfigurationEntry conf) {
+ term.set(conf.getId().getTerm());
+ index.set(conf.getId().getIndex());
+ super.onRawConfigurationCommitted(conf);
+ }
+ });
+ },
+ testInfo
+ );
+
+ assertTrue(cluster.start(peer0));
+
+ cluster.ensureLeader(cluster.waitAndGetLeader());
+
+ Node leader = cluster.waitAndGetLeader();
+
+ assertNotEquals(-1, term.get());
+ assertNotEquals(-1, index.get());
+
+ term.set(-1);
+ index.set(-1);
+
+ sendTestTaskAndWait(leader);
+
+ TestPeer newPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 1);
+ assertTrue(cluster.start(newPeer, false, 300));
+
+ verify(raftGrpEvtsLsnr, never()).onNewPeersConfigurationApplied(any(),
any(), anyLong(), anyLong());
+
+ // Wait until new node node sees every other node, otherwise
+ // changePeersAndLearnersAsync can fail.
+ waitForTopologyOnEveryNode(1, cluster);
+
+ leader = cluster.getLeader();
+ assertNotNull(leader);
+ assertEquals(peer0.getPeerId(), leader.getNodeId().getPeerId());
+
+ SynchronizedClosure done = new SynchronizedClosure();
+ leader.changePeersAndLearnersAsync(new
Configuration(List.of(peer0.getPeerId(), newPeer.getPeerId()), List.of()),
leader.getCurrentTerm(), done);
+
+ assertEquals(done.await(), Status.OK());
+
+ assertTrue(waitForCondition(() -> {
+ if (cluster.getLeader() != null) {
+ return
cluster.getLeader().listAlivePeers().contains(newPeer.getPeerId());
+ }
+ return false;
+ }, 10_000));
+
+ verify(
+ raftGrpEvtsLsnr,
+
times(1)).onNewPeersConfigurationApplied(List.of(peer0.getPeerId(),
newPeer.getPeerId()), List.of(), term.get(), index.get()
Review Comment:
How do you check proper values of term and index?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
