ivandika3 commented on code in PR #1589:
URL: https://github.com/apache/ratis/pull/1589#discussion_r4035033992
##########
ratis-server-api/src/main/java/org/apache/ratis/server/RaftServer.java:
##########
@@ -82,6 +83,27 @@ default RaftPeer getPeer() {
/** @return the information about this division. */
DivisionInfo getInfo();
+ /**
+ * Create a {@link NotLeaderException} using the current division state.
+ *
+ * <p>The suggested leader and peers are best-effort hints. The suggested
+ * leader is null if this division is not running or if its current leader
+ * ID is unknown or is the ID of this division.</p>
+ */
+ default NotLeaderException newNotLeaderException() {
+ if (!getInfo().getLifeCycleState().isRunning()) {
+ return new NotLeaderException(getMemberId(), null, null);
+ }
+ RaftPeerId leaderId = getInfo().getLeaderId();
+ if (leaderId == null || leaderId.equals(getId())) {
+ // No idea about who is the current leader. Or the peer is the current
+ // leader, but it is about to step down. set the suggested leader as
null.
Review Comment:
> I am wondering if we only need a single new API as checkLeaderReady as a
general use case. A caller will always call checkLeaderReady
Most of the time the user should use exception returned in
`checkLeaderReady` (or null if the current peer is a leader), but
`newNotLeaderException` might still needed for some custom use cases like when
the server is stopping (similar to the `NotLeaderException` generated in
`LeaderStateImpl#stop`). Also if we remove `newNotLeaderException`, the
`Division#checkLeaderReady` cannot be a default method and we need the
subclasses to implement this which can introduce the duplicate logic we are
trying to avoid.
If you think that `checkLeaderReady` is enough, I can remove it.
##########
ratis-server-api/src/main/java/org/apache/ratis/server/RaftServer.java:
##########
@@ -82,6 +83,27 @@ default RaftPeer getPeer() {
/** @return the information about this division. */
DivisionInfo getInfo();
+ /**
+ * Create a {@link NotLeaderException} using the current division state.
+ *
+ * <p>The suggested leader and peers are best-effort hints. The suggested
+ * leader is null if this division is not running or if its current leader
+ * ID is unknown or is the ID of this division.</p>
+ */
+ default NotLeaderException newNotLeaderException() {
+ if (!getInfo().getLifeCycleState().isRunning()) {
+ return new NotLeaderException(getMemberId(), null, null);
+ }
+ RaftPeerId leaderId = getInfo().getLeaderId();
+ if (leaderId == null || leaderId.equals(getId())) {
+ // No idea about who is the current leader. Or the peer is the current
+ // leader, but it is about to step down. set the suggested leader as
null.
Review Comment:
> I am wondering if we only need a single new API as checkLeaderReady as a
general use case. A caller will always call checkLeaderReady
Most of the time the user should use exception returned in
`checkLeaderReady` (or null if the current peer is a leader), but
`newNotLeaderException` might still needed for some custom use cases like when
the server is stopping (similar to the `NotLeaderException` generated in
`LeaderStateImpl#stop`). Also if we remove `newNotLeaderException`, the
`Division#checkLeaderReady` cannot be a default method and we need the
subclasses to implement this which can introduce the duplicate logic we are
trying to avoid.
If you think that `checkLeaderReady` is enough, I can remove
`newNotLeaderException`.
--
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]
