RATIS-2055. Move notifyTermIndexUpdated after leader.checkReady#1068
RATIS-2055. Move notifyTermIndexUpdated after leader.checkReady#1068szetszwo merged 4 commits intoapache:masterfrom
Conversation
|
@szetszwo @duongkame Raised the PR based on the PR in ticket. PTAL. |
szetszwo
left a comment
There was a problem hiding this comment.
@symious , thanks for working on this! Please see the comments inline and below
+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
@@ -1813,7 +1813,6 @@ class RaftServerImpl implements RaftServer.Division,
stateMachine.event().notifyConfigurationChanged(next.getTerm(), next.getIndex(),
next.getConfigurationEntry());
role.getLeaderState().ifPresent(leader -> leader.checkReady(next));
- stateMachine.event().notifyTermIndexUpdated(next.getTerm(), next.getIndex());
break;
case STATEMACHINELOGENTRY:
TransactionContext trx = getTransactionContext(next, true);
@@ -1829,9 +1828,14 @@ class RaftServerImpl implements RaftServer.Division,
} catch (Exception e) {
throw new RaftLogIOException(e);
}
+ case METADATAENTRY:
+ break;
default:
- stateMachine.event().notifyTermIndexUpdated(next.getTerm(), next.getIndex());
+ throw new IllegalStateException("Unexpected LogEntryBodyCase " + next.getLogEntryBodyCase() + ", next=" + next);
}
+
+ // for non-STATEMACHINELOGENTRY cases
+ stateMachine.event().notifyTermIndexUpdated(next.getTerm(), next.getIndex());
return null;
}| } | ||
|
|
||
| if (next.hasConfigurationEntry()) { | ||
| switch (next.getLogEntryBodyCase()) { |
There was a problem hiding this comment.
It is good to use switch. Let add also case METADATAENTRY and make default throw an exception.
| next.getConfigurationEntry()); | ||
| role.getLeaderState().ifPresent(leader -> leader.checkReady(next)); | ||
| } else if (next.hasStateMachineLogEntry()) { | ||
| stateMachine.event().notifyTermIndexUpdated(next.getTerm(), next.getIndex()); |
There was a problem hiding this comment.
Let move it down to right before return null. Then, we don't have to repeat it for each case.
There was a problem hiding this comment.
@szetszwo Thank you for the review.
I realized that I repeat it because I missed the point that StateMachineLogEntry will return inside the case. I changed this part also for a better readability.
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
What changes were proposed in this pull request?
Move notifyTermIndexUpdated after leader.checkReady
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2055
How was this patch tested?
Existing tests.