You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "amaliujia (via GitHub)" <gi...@apache.org> on 2026/10/17 14:05:19 UTC
[PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
amaliujia opened a new pull request, #1647:
URL: https://github.com/apache/ratis/pull/1647
## What changes were proposed in this pull request?
Notify the StateMachine when a server shutdown starts
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2721
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
Unit test
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
/**
* Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
*
* @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
/** ... existing Javadoc ... */
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4333380385
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {
+ notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link TaskStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link TaskStatus#INITIATED}.
+ * <p>
+ * {@link TaskStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
* @param allServer whether all raft servers will be shutdown at this time
+ * @param taskStatus {@link TaskStatus#INITIATED} or {@link TaskStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer, TaskStatus taskStatus) {}
Review Comment:
done. Also changed for existing API. As far as I know, change parameter name does not break compatibility in Java.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4356268082
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -604,14 +613,14 @@ private void closeImpl(Throwable cause) {
failure = e;
throw e;
} finally {
- completeClose(failure);
+ completeClose(failure, allServers);
}
}
- private void completeClose(Throwable cause) {
+ private void completeClose(Throwable cause, boolean allServers) {
lifeCycle.transitionIfNotEqual(State.CLOSED);
if (cause == null) {
- closeFuture.complete(null);
+ closeFuture.complete(allServers);
} else {
closeFuture.completeExceptionally(cause);
}
Review Comment:
True. We in fact does nothing in the whenComplete clause for exceptional cases.
I changed to just call `closeFuture.complete(allServers);` here but also added a TODO to document that we do not actually handle `cause` yet.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
This might not be that perfect though if a user calls `notifyServerShutdown(roleInfo, allServers)`, the user might expect the implementation for `notifyServerShutdown(roleInfo, allServers, eventStatus)` will be invoked, however it will not. The root cause of the confusion still is from we do not throw unsupported exception but choose to have a default no-op. Anyway the above is the best I can see now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4320704544
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -125,6 +125,9 @@ public synchronized void close() {
return;
}
isClosed = true;
+ // Notify all the state machines upfront: the close tasks submitted below each wait for the committed entries
+ // to be applied, so a task queued behind them could otherwise be delayed for an unbounded time.
+ map.values().forEach(f -> f.thenAccept(impl -> impl.notifyServerShutdownInitiated(true)));
Review Comment:
The subsequent `close` under `parallelForEachAsync` also called `notifyServerShutdownInitiated`, is this a duplicate?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4351287179
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -299,10 +300,12 @@ public long[] getFollowerMatchIndices() {
id + "-client");
this.threadGroup = new ThreadGroup(proxy.getThreadGroup(), getMemberId().toString());
- this.closeFuture.whenComplete((v, e) -> {
+ this.closeFuture.whenComplete((allServers, e) -> {
// A division closed before its role is initialized has no role info to report.
if (role.getCurrentRole() != null) {
- stateMachine.event().notifyServerShutdown(getRoleInfoProto(), false);
+ // allServers is null when the close has failed.
Review Comment:
The current code in some cases simply call close() but not close(cause) when there is an exception. If we add EventStatus.FAILED, we should also fix these cases.
It seems to me that it does not worth the effort for now. If we really want to do it, we can change it in a separated JIRA.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367392161
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
If the user does not implement these functions but happen to call `notifyServerShutdown(roleInfo, allServers)`, it will turn into an infinite loop rather than a harmless no-op.
One approach is we actually throw exception in the default implementation but that would be a behavior change for the existing API.
As of now I feel like the better way is we do not migrate existing `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)` calls to the EventStatus based calls, which then will preserve existing behavior:
1. so the complete status report still uses `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)`
2. only the initialized status report uses `default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus)`.
I will adopt this way for now.
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
If the user does not implement these functions but happen to call `notifyServerShutdown(roleInfo, allServers)`, it will turn into an infinite loop rather than a harmless no-op.
One approach is we actually throw unsupported exception in the default implementation but that would be a behavior change for the existing API.
As of now I feel like the better way is we do not migrate existing `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)` calls to the EventStatus based calls, which then will preserve existing behavior:
1. so the complete status report still uses `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)`
2. only the initialized status report uses `default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus)`.
I will adopt this way for now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367392161
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
If the user does not implement these functions but happen to call `notifyServerShutdown(roleInfo, allServers);`, it will turn into an infinite loop rather than a harmless no-op.
One approach is we actually throw exception in the default implementation but that would be a behavior change for the existing API.
As of now I feel like the better way is we do not migrate existing `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)` calls to the EventStatus based calls, which then will preserve existing behavior:
1. so the complete status report still uses `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)`
2. only the initialized status report uses `default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus)`.
I will adopt this way for now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1647:
URL: https://github.com/apache/ratis/pull/1647
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4351337861
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -604,14 +613,14 @@ private void closeImpl(Throwable cause) {
failure = e;
throw e;
} finally {
- completeClose(failure);
+ completeClose(failure, allServers);
}
}
- private void completeClose(Throwable cause) {
+ private void completeClose(Throwable cause, boolean allServers) {
lifeCycle.transitionIfNotEqual(State.CLOSED);
if (cause == null) {
- closeFuture.complete(null);
+ closeFuture.complete(allServers);
} else {
closeFuture.completeExceptionally(cause);
}
Review Comment:
@amaliujia , For completeExceptionally(..), we will lose the allServers information. How about calling
complete(allServers) even for exceptions? The exception information currently is unused.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4368188510
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
> Ah actually I guess the complete solution is this...
Agree.
> ... if a user calls notifyServerShutdown(roleInfo, allServers), ...
The notification methods should only be called by Ratis but not the users.
- BTW, when we add the allServers parameter, we should implement it the other way as below. Let's also fix it here.
```java
default void notifyServerShutdown(RoleInfoProto roleInfo) {}
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
notifyServerShutdown(roleInfo);
}
```
- Let's also change the javadoc: "please use/override" -> "please override". It is more clear.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4345232947
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -299,10 +300,12 @@ public long[] getFollowerMatchIndices() {
id + "-client");
this.threadGroup = new ThreadGroup(proxy.getThreadGroup(), getMemberId().toString());
- this.closeFuture.whenComplete((v, e) -> {
+ this.closeFuture.whenComplete((allServers, e) -> {
// A division closed before its role is initialized has no role info to report.
if (role.getCurrentRole() != null) {
- stateMachine.event().notifyServerShutdown(getRoleInfoProto(), false);
+ // allServers is null when the close has failed.
Review Comment:
@szetszwo do you think if we actually need a
```
if (allServers == null) || e != null) {
// add a status EventStatus.FAILED and post this status?
}
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4303235487
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -243,6 +243,33 @@ default void notifyServerShutdown(RoleInfoProto roleInfo) {
* @param allServer whether all raft servers will be shutdown at this time
*/
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+
+ /**
+ * Notify the {@link StateMachine} that the shutdown of the server for this division has started.
+ * @deprecated please use/override {@link #notifyServerShutdownStarted(RoleInfoProto, boolean)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdownStarted(RoleInfoProto roleInfo) {
+ notifyServerShutdownStarted(roleInfo, false);
+ }
+
+ /**
+ * Notify the {@link StateMachine} that the shutdown of either the server for this division
+ * or all the servers has started.
+ * <p>
+ * In contrast to {@link #notifyServerShutdown(RoleInfoProto, boolean)}, which is invoked once the shutdown
+ * has completed, this method is invoked before the shutdown has closed any resources.
+ * In particular, it is invoked before waiting for the committed log entries to be applied to this
+ * {@link StateMachine}, which is unbounded in time. An application embedding a {@link RaftServer} can use
+ * this notification to react immediately, e.g. to reject incoming requests or to terminate itself,
+ * instead of waiting for the shutdown to complete.
+ * <p>
+ * This method is invoked from the shutdown path, so an implementation must return promptly.
+ *
+ * @param roleInfo roleInfo this server
+ * @param allServer whether all raft servers will be shutdown at this time
+ */
+ default void notifyServerShutdownStarted(RoleInfoProto roleInfo, boolean allServer) {}
Review Comment:
@amaliujia , thanks for working on this!
Please deprecate and override the existing `notifyServerShutdown` method:
(revision: renamed the new enum to EventStatus)
```diff
diff --git a/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java b/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
index bd009b0e9..967debb04 100644
--- a/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
+++ b/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
@@ -58,6 +58,11 @@ import java.util.function.Function;
public interface StateMachine extends Closeable {
Logger LOG = LoggerFactory.getLogger(StateMachine.class);
+ enum EventStatus {
+ INITIATED,
+ COMPLETED
+ }
+
/** A registry to support different state machines in multi-raft environment. */
interface Registry extends Function<RaftGroupId, StateMachine> {
}
@@ -229,20 +234,33 @@ public interface StateMachine extends Closeable {
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServer, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {
+ notifyServerShutdown(roleInfo, allServer, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ *
* @param roleInfo roleInfo this server
* @param allServer whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer, EventStatus eventStatus) {}
}
/**
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
/**
* Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
*
* @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
/** ... existing Javadoc ... */
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
This might not be that perfect though if a user calls `notifyServerShutdown(roleInfo, allServers)`, the user might expect the implementation for `notifyServerShutdown(roleInfo, allServers, eventStatus)` will be invoked, however it will not. The root cause of the confusion still is from we do not throw unsupported exception but choose to have a default no-op. Anyway the above is the best I can see now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
/**
* Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
*
* @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
/** ... existing Javadoc ... */
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
This might not be that perfect though if a user calls `notifyServerShutdown(roleInfo, allServers)`, the user might expect the implementation for `notifyServerShutdown(roleInfo, allServers, eventStatus)` will be invoked, however it will not. The root cause of the confusion still is from we do not throw unsupported exception but choose to have a default no-op, anyway the above is the best I can see now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4333404217
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -125,6 +125,9 @@ public synchronized void close() {
return;
}
isClosed = true;
+ // Notify all the state machines upfront: the close tasks submitted below each wait for the committed entries
+ // to be applied, so a task queued behind them could otherwise be delayed for an unbounded time.
+ map.values().forEach(f -> f.thenAccept(impl -> impl.notifyServerShutdownInitiated(true)));
Review Comment:
I was thinking about if `close` used by `parallelForEachAsync` runs in a limited thread pool so the event was not able to send out sooner . So a up-front call might be able to accelerate this.
But this maybe just over-kill. I ended up with removing this up-front call. The minimal we need is just to pair with those post shutdown events.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367392161
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
If the user does not implement these functions but happen to call `notifyServerShutdown(roleInfo, allServers);`, it will turn into an infinite loop.
As of now I feel like the better way is we do not migrate existing `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)` calls to the EventStatus based calls, which then will preserve existing behavior:
1. so the complete status report still uses `notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer)`
2. only the initialized status report uses `default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus)`.
I will update to this way for now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4333404217
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -125,6 +125,9 @@ public synchronized void close() {
return;
}
isClosed = true;
+ // Notify all the state machines upfront: the close tasks submitted below each wait for the committed entries
+ // to be applied, so a task queued behind them could otherwise be delayed for an unbounded time.
+ map.values().forEach(f -> f.thenAccept(impl -> impl.notifyServerShutdownInitiated(true)));
Review Comment:
I was thinking about if `close` used `parallelForEachAsync` runs in a limited thread pool so the event was not able to send out sooner. So a up-front call might be able to accelerate this.
But this maybe just over-kill. I ended up with removing this up-front call.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1647:
URL: https://github.com/apache/ratis/pull/1647#issuecomment-6322124861
@amaliujia , thanks for the update!
> - [RATIS-2645](https://issues.apache.org/jira/browse/RATIS-2645) added a close(cause) method. We could notify INITIATED there
> - We only need to notify COMPLETED when the close future completes.
>
> See https://issues.apache.org/jira/secure/attachment/13085559/1647_review.patch
We only need to notify INITIATED and COMPLETED respectively in close(..) and closeFuture.whenComplete(..). Please take a look at 1647_review.patch
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4356538470
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -604,14 +613,14 @@ private void closeImpl(Throwable cause) {
failure = e;
throw e;
} finally {
- completeClose(failure);
+ completeClose(failure, allServers);
}
}
- private void completeClose(Throwable cause) {
+ private void completeClose(Throwable cause, boolean allServers) {
lifeCycle.transitionIfNotEqual(State.CLOSED);
if (cause == null) {
- closeFuture.complete(null);
+ closeFuture.complete(allServers);
} else {
closeFuture.completeExceptionally(cause);
}
Review Comment:
I actually reverted this suggestion. Found test failures which indicates that, this closeFuture.completeExceptionally is used by somewhere else: https://github.com/apache/ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java#L486-L495
test failures from this job: https://github.com/apache/ratis/actions/runs/39903654392/job/120250580159?pr=1647
So basically there is other code depending on this `closeFuture.completeExceptionally` so it cannot be dropped. Then if we do not want to lose `allServers` here, probably one way is to wrap it into our exception. WDYT?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4363121278
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -604,14 +613,14 @@ private void closeImpl(Throwable cause) {
failure = e;
throw e;
} finally {
- completeClose(failure);
+ completeClose(failure, allServers);
}
}
- private void completeClose(Throwable cause) {
+ private void completeClose(Throwable cause, boolean allServers) {
lifeCycle.transitionIfNotEqual(State.CLOSED);
if (cause == null) {
- closeFuture.complete(null);
+ closeFuture.complete(allServers);
} else {
closeFuture.completeExceptionally(cause);
}
Review Comment:
Good catch on the problem!
The closeFuture was newly added by RATIS-2645. For simplicity, let's keep the current change.
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
We actually need to implement it in order to preserve the existing behavior:
```java
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
/**
* Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
*
* @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
/** ... existing Javadoc ... */
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
This might be that perfect though if a user calls `notifyServerShutdown(roleInfo, allServers)`. the user might expect the implementation for `notifyServerShutdown(roleInfo, allServers, eventStatus)` will be invoked, however it will not. The root cause of the confusion still is from we do not throw unsupported exception but choose to have a default no-op, anyway the above is the best I can see now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4367406105
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
Ah actually I guess the complete solution is this:
```
/**
* Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
*
* @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {}
/** ... existing Javadoc ... */
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {
// preserve the existing behavior
if (eventStatus == EventStatus.COMPLETED) {
notifyServerShutdown(roleInfo, allServers);
}
}
```
It became a bit confusing to me...
This might not be that perfect though if a user calls `notifyServerShutdown(roleInfo, allServers)`. the user might expect the implementation for `notifyServerShutdown(roleInfo, allServers, eventStatus)` will be invoked, however it will not. The root cause of the confusion still is from we do not throw unsupported exception but choose to have a default no-op, anyway the above is the best I can see now.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4316416583
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -58,6 +58,11 @@
public interface StateMachine extends Closeable {
Logger LOG = LoggerFactory.getLogger(StateMachine.class);
+ enum TaskStatus {
Review Comment:
Sorry, let's call it `EventStatus` since it is for the EventApi.
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {
+ notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link TaskStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link TaskStatus#INITIATED}.
+ * <p>
+ * {@link TaskStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
* @param allServer whether all raft servers will be shutdown at this time
+ * @param taskStatus {@link TaskStatus#INITIATED} or {@link TaskStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer, TaskStatus taskStatus) {}
Review Comment:
It is better to rename `allServer` to `allServers`. Let's take this chance to fix it.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1647:
URL: https://github.com/apache/ratis/pull/1647#issuecomment-6273590220
Cc @ivandika3
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4303235487
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -243,6 +243,33 @@ default void notifyServerShutdown(RoleInfoProto roleInfo) {
* @param allServer whether all raft servers will be shutdown at this time
*/
default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+
+ /**
+ * Notify the {@link StateMachine} that the shutdown of the server for this division has started.
+ * @deprecated please use/override {@link #notifyServerShutdownStarted(RoleInfoProto, boolean)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdownStarted(RoleInfoProto roleInfo) {
+ notifyServerShutdownStarted(roleInfo, false);
+ }
+
+ /**
+ * Notify the {@link StateMachine} that the shutdown of either the server for this division
+ * or all the servers has started.
+ * <p>
+ * In contrast to {@link #notifyServerShutdown(RoleInfoProto, boolean)}, which is invoked once the shutdown
+ * has completed, this method is invoked before the shutdown has closed any resources.
+ * In particular, it is invoked before waiting for the committed log entries to be applied to this
+ * {@link StateMachine}, which is unbounded in time. An application embedding a {@link RaftServer} can use
+ * this notification to react immediately, e.g. to reject incoming requests or to terminate itself,
+ * instead of waiting for the shutdown to complete.
+ * <p>
+ * This method is invoked from the shutdown path, so an implementation must return promptly.
+ *
+ * @param roleInfo roleInfo this server
+ * @param allServer whether all raft servers will be shutdown at this time
+ */
+ default void notifyServerShutdownStarted(RoleInfoProto roleInfo, boolean allServer) {}
Review Comment:
@amaliujia , thanks for working on this!
Please deprecate and override the existing `notifyServerShutdown` method:
```diff
diff --git a/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java b/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
index bd009b0e9..d12cc3f3f 100644
--- a/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
+++ b/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java
@@ -58,6 +58,11 @@ import java.util.function.Function;
public interface StateMachine extends Closeable {
Logger LOG = LoggerFactory.getLogger(StateMachine.class);
+ enum TaskStatus {
+ INITIATED,
+ COMPLETED
+ }
+
/** A registry to support different state machines in multi-raft environment. */
interface Registry extends Function<RaftGroupId, StateMachine> {
}
@@ -229,20 +234,33 @@ public interface StateMachine extends Closeable {
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, TaskStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, TaskStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {
+ notifyServerShutdown(roleInfo, allServer, TaskStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ *
* @param roleInfo roleInfo this server
* @param allServer whether all raft servers will be shutdown at this time
+ * @param taskStatus {@link TaskStatus#INITIATED} or {@link TaskStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer, TaskStatus taskStatus) {}
}
/**
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4333404217
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerProxy.java:
##########
@@ -125,6 +125,9 @@ public synchronized void close() {
return;
}
isClosed = true;
+ // Notify all the state machines upfront: the close tasks submitted below each wait for the committed entries
+ // to be applied, so a task queued behind them could otherwise be delayed for an unbounded time.
+ map.values().forEach(f -> f.thenAccept(impl -> impl.notifyServerShutdownInitiated(true)));
Review Comment:
I was thinking about if `close` used `parallelForEachAsync` runs in a limited thread pool so the event was not able to send out sooner . So a up-front call might be able to accelerate this.
But this maybe just over-kill. I ended up with removing this up-front call. The minimal we need is just to pair with those post shutdown events.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2721. Notify the StateMachine when a server shutdown starts [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1647:
URL: https://github.com/apache/ratis/pull/1647#discussion_r4371085610
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -229,20 +234,39 @@ default void notifyLogFailed(Throwable cause, LogEntryProto failedEntry) {}
default void notifySnapshotInstalled(InstallSnapshotResult result, long snapshotIndex, RaftPeer peer) {}
/**
- * Notify the {@link StateMachine} that the server for this division has been shut down.
- * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean)} instead
+ * The same as notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
*/
@Deprecated
default void notifyServerShutdown(RoleInfoProto roleInfo) {
- notifyServerShutdown(roleInfo, false);
+ notifyServerShutdown(roleInfo, false, EventStatus.COMPLETED);
+ }
+
+ /**
+ * The same as notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED).
+ *
+ * @deprecated please use/override {@link #notifyServerShutdown(RoleInfoProto, boolean, EventStatus)} instead
+ */
+ @Deprecated
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers) {
+ notifyServerShutdown(roleInfo, allServers, EventStatus.COMPLETED);
}
/**
- * Notify the {@link StateMachine} that either the server for this division or all the servers have been shut down.
+ * Notify the {@link StateMachine} server shutdown.
+ * <p>
+ * {@link EventStatus#INITIATED} is notified before the shutdown has closed any resources,
+ * in particular, before waiting for the committed log entries to be applied to this {@link StateMachine},
+ * which is unbounded in time. An implementation must return promptly for {@link EventStatus#INITIATED}.
+ * <p>
+ * {@link EventStatus#COMPLETED} is notified after the shutdown has completed.
+ *
* @param roleInfo roleInfo this server
- * @param allServer whether all raft servers will be shutdown at this time
+ * @param allServers whether all raft servers will be shutdown at this time
+ * @param eventStatus {@link EventStatus#INITIATED} or {@link EventStatus#COMPLETED}
*/
- default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServer) {}
+ default void notifyServerShutdown(RoleInfoProto roleInfo, boolean allServers, EventStatus eventStatus) {}
Review Comment:
I see. Done
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org