alievmirza commented on code in PR #3404:
URL: https://github.com/apache/ignite-3/pull/3404#discussion_r1523220732
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -153,6 +151,8 @@ public void testSafeTimeReorderingOnLeaderReElection()
throws Exception {
RaftGroupService anotherClient = aliveNode.get().raftClient;
+ assertThat(anotherClient.refreshLeader(), willCompleteSuccessfully());
Review Comment:
Why do we need to add this assertion? The same question for the other rest
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/FSMCallerImpl.java:
##########
@@ -283,6 +283,12 @@ public boolean onLeaderStop(final Status status) {
});
}
+ @Override
+ public boolean onBeforeLeaderStart() {
+ doBeforeLeaderStart();
Review Comment:
It seems to me that the fact, that we call `doBeforeLeaderStart()` directly,
but not through the `enqueueTask` mechanism, which is done for all leader
callbacks, could lead to to the linearisability problems. For example, when we
transfer leadership, we call
`org.apache.ignite.raft.jraft.core.NodeImpl#onLeaderStop` , but if this
transferring timeouts, we return back the old leader, and we expect that
`onBeforeLeaderStart` will be called, but because we call it directly,
`onLeaderStop` could be invoked after `onBeforeLeaderStart`
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -1414,6 +1414,7 @@ else if (this.leaderId.isEmpty()) {
}
private void becomeLeader() {
+ this.fsmCaller.onBeforeLeaderStart();
Review Comment:
This is not the only place where node could become a leader.
See `org.apache.ignite.raft.jraft.core.NodeImpl#handleTransferTimeout`
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -1414,6 +1414,7 @@ else if (this.leaderId.isEmpty()) {
}
private void becomeLeader() {
+ this.fsmCaller.onBeforeLeaderStart();
Review Comment:
If we want to call it here, we must call it after
```
Requires.requireTrue(this.state == State.STATE_CANDIDATE, "Illegal state: "
+ this.state);
```
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/StateMachine.java:
##########
@@ -67,6 +67,12 @@ public interface StateMachine {
*/
boolean onSnapshotLoad(final SnapshotReader reader);
+ /**
+ * Invoked when the belonging node is about to become the leader of the
group. Default: Do nothing
+ */
+ void onBeforeLeaderStart();
+
Review Comment:
Redundant line
--
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]
