Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RATIS-1874 Add notifyLeaderReady function in IStateMachine #906

Merged
merged 3 commits into from
Aug 25, 2023
Merged

RATIS-1874 Add notifyLeaderReady function in IStateMachine #906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45b1a65
finish
OneSizeFitsQuorum Aug 23, 2023
e972601
fix wrong index
OneSizeFitsQuorum Aug 23, 2023
cc0a2c1
fix review
OneSizeFitsQuorum Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ default void notifyFollowerSlowness(RoleInfoProto leaderInfo) {}
* Notify {@link StateMachine} that this server is no longer the leader.
*/
default void notifyNotLeader(Collection<TransactionContext> pendingEntries) throws IOException {}

/**
* Notify the {@link StateMachine} that this server becomes ready after changed to leader.
*/
default void notifyLeaderReady() {}
Copy link
Member

@tisonkun tisonkun Sep 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method name can be a bit misleading -

  • What I read: a leader is ready for this group.
  • What it actually means: this server is ready after changed to leader.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps notifyServingAsLeader or notifyBecomeLeader

Copy link
Contributor Author

@OneSizeFitsQuorum OneSizeFitsQuorum Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What it actually means: this server is reader after changed to leader.

Is reader a typo for ready?

This function means that the current server has become a leader and has fully recovered the state machine (at least one log entry from the current term has been applied).

If we use the name notifyBecomeLeader, it seems to suggest that the current peer has been elected as a leader but doesn't explicitly convey the semantics of the state machine recovery. notifyServingAsLeader and notifyLeaderReady both imply some level of state machine recovery in my view, and either of them could work for me.

What's your opinion? @szetszwo @tisonkun @SzyWilliam

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a notifyLeaderChanged(..) method in EventApi. So, both the Leader and the Followers will get this event. Of course, the server could check if it becomes the leader by checking the newLeaderId parameter.

This method LeaderEventApi.notifyLeaderReady() is in LeaderEventApi. So, only the Leader will get this event. The name looks good to me. Also, the javadoc is clear.

    /**
     * Notify the {@link StateMachine} that this server becomes ready after changed to leader.
     */
    default void notifyLeaderReady() {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the javadoc is clear

This is a good point. The method name is conveyed by LeaderStateImpl#isReady with its doc clear. So notifyLeaderReady should be a good name follow this idiom >_<

}

/**
Expand Down
10 changes: 9 additions & 1 deletion ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ private List<FollowerInfo> getFollowerInfos(PeerConfiguration peers) {

private final int stagingCatchupGap;
private final long placeHolderIndex;
private final AtomicBoolean isReady = new AtomicBoolean();
private final RaftServerMetricsImpl raftServerMetrics;
private final LogAppenderMetrics logAppenderMetrics;
private final long followerMaxGapThreshold;
Expand Down Expand Up @@ -383,7 +384,14 @@ LogEntryProto start() {
}

boolean isReady() {
return server.getState().getLastAppliedIndex() >= placeHolderIndex;
return isReady.get();
}

void checkReady(LogEntryProto entry) {
if (entry.getTerm() == currentTerm && entry.getIndex() == placeHolderIndex) {
isReady.set(true);
server.getStateMachine().leaderEvent().notifyLeaderReady();
}
}

void stop() {
Expand Down
1 change: 1 addition & 0 deletions ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1802,6 +1802,7 @@ CompletableFuture<Message> applyLogToStateMachine(LogEntryProto next) throws Raf
// the new conf in the metadata file and notify the StateMachine.
state.writeRaftConfiguration(next);
stateMachine.event().notifyConfigurationChanged(next.getTerm(), next.getIndex(), next.getConfigurationEntry());
role.getLeaderState().ifPresent(leader -> leader.checkReady(next));
} else if (next.hasStateMachineLogEntry()) {
// check whether there is a TransactionContext because we are the leader.
TransactionContext trx = role.getLeaderState()
Expand Down
Loading