You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/04/29 04:39:44 UTC
[GitHub] [ratis] inomp3042 opened a new pull request, #638: Add follower information for leader to know follower when follower is slow
inomp3042 opened a new pull request, #638:
URL: https://github.com/apache/ratis/pull/638
## What changes were proposed in this pull request?
When notifying follower's slowness, if the leader knows who is slow using StateMachine, then it will help users to handle 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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861578865
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
I see. thanks!
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861598340
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
I'm just wondering about your comment.
FollowerInfo is in ratis-server-api project's leader package, and `notifyFollowerSlowness`, which is in `LeaderEventApi`, is called on a leader. Even though it is in this case, should it not appear in the StateMachine API?
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861925153
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
> ... How about marking the follower as slow in RoleInfoProto?
Sorry, this idea is incorrect. The RoleInfoProto is for leader. We should not change it.
How about adding a RaftPeerId for the slow follower?
```
default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, RaftPeerId slowFollower) {}
```
--
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
[GitHub] [ratis] szetszwo merged pull request #638: RATIS-1574. Add follower information to LeaderEventApi.notifyFollowerSlowness(..)
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #638:
URL: https://github.com/apache/ratis/pull/638
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861553997
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
@inomp3042 , thanks for working on this. Since FollowerInfo is in the Leader package, it should not appear in the StateMachine API. How about marking the follower as slow in RoleInfoProto?
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861598340
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
However, FollowerInfo is in ratis-server-api project's leader package, and `notifyFollowerSlowness`, which is in `LeaderEventApi`, is called on a leader. Even though it is in this case, should it not appear in the StateMachine API?
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862676324
##########
ratis-server/src/test/java/org/apache/ratis/statemachine/SimpleStateMachine4Testing.java:
##########
@@ -416,9 +417,15 @@ public void unblockFlushStateMachineData() {
}
@Override
- public void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {
- LOG.info("{}: notifySlowness {}, {}", this, groupId, roleInfoProto);
- slownessInfo = roleInfoProto;
+ public void notifyFollowerSlowness(RoleInfoProto leaderInfo) {
+ LOG.info("{}: notifySlowness {}, {}", this, groupId, leaderInfo);
+ slownessInfo = leaderInfo;
+ }
Review Comment:
I'm not sure that keeping the deprecated method is okay. Please let me know if you have any opinions. It seems that `slownessInfo` is used in others.. so It can disturb the unit tests until removing the deprecated method.
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862606942
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
Sure, RaftPeer is good.
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862749237
##########
ratis-server/src/test/java/org/apache/ratis/statemachine/SimpleStateMachine4Testing.java:
##########
@@ -416,9 +417,15 @@ public void unblockFlushStateMachineData() {
}
@Override
- public void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {
- LOG.info("{}: notifySlowness {}, {}", this, groupId, roleInfoProto);
- slownessInfo = roleInfoProto;
+ public void notifyFollowerSlowness(RoleInfoProto leaderInfo) {
+ LOG.info("{}: notifySlowness {}, {}", this, groupId, leaderInfo);
+ slownessInfo = leaderInfo;
+ }
Review Comment:
Please remove notifyFollowerSlowness(RoleInfoProto leaderInfo) since it already has notifyFollowerSlowness(RoleInfoProto leaderInfo, RaftPeer slowFollower). Implementations only have to override one of the methods.
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862676324
##########
ratis-server/src/test/java/org/apache/ratis/statemachine/SimpleStateMachine4Testing.java:
##########
@@ -416,9 +417,15 @@ public void unblockFlushStateMachineData() {
}
@Override
- public void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {
- LOG.info("{}: notifySlowness {}, {}", this, groupId, roleInfoProto);
- slownessInfo = roleInfoProto;
+ public void notifyFollowerSlowness(RoleInfoProto leaderInfo) {
+ LOG.info("{}: notifySlowness {}, {}", this, groupId, leaderInfo);
+ slownessInfo = leaderInfo;
+ }
Review Comment:
I'm not sure that keeping the deprecated method is okay. Please let me know if you have any opinions.
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862665038
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -207,7 +208,7 @@ interface LeaderEventApi {
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
Review Comment:
Ah, Thanks. I missed that.
--
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
[GitHub] [ratis] szetszwo commented on pull request #638: RATIS-1574. Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #638:
URL: https://github.com/apache/ratis/pull/638#issuecomment-1115792869
@inomp3042 , great. Thanks!
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861918937
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
FollowerInfo is for LogAppender implementation but not StateMachine implementation. For example, we don't want a StateMachine implementation calls FollowerInfo.setNextIndex(..).
--
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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862606041
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
> Sorry, this idea is incorrect. The RoleInfoProto is for leader. We should not change it.
How about adding a RaftPeerId for the slow follower?
I see. I think that RaftPeer is better than RaftPeerId. 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
[GitHub] [ratis] inomp3042 commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r861598340
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -204,10 +205,11 @@ interface LeaderEventApi {
* This notification is based on "raft.server.rpc.slowness.timeout".
*
* @param roleInfoProto information about the current node role and rpc delay information
+ * @param followerInfo information about slow follower
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
+ default void notifyFollowerSlowness(RoleInfoProto roleInfoProto, FollowerInfo followerInfo) {}
Review Comment:
However, FollowerInfo is in ratis-server-api project's leader package, and `notifyFollowerSlowness`, which is in `LeaderEventApi`, is called on a leader. Even though it is in this case, it should not appear in the StateMachine API?
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #638:
URL: https://github.com/apache/ratis/pull/638#discussion_r862650331
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -207,7 +208,7 @@ interface LeaderEventApi {
*
* @see org.apache.ratis.server.RaftServerConfigKeys.Rpc#SLOWNESS_TIMEOUT_KEY
*/
- default void notifyFollowerSlowness(RoleInfoProto roleInfoProto) {}
Review Comment:
We should first deprecate the API and remove it in a later version.
--
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
[GitHub] [ratis] szetszwo commented on pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #638:
URL: https://github.com/apache/ratis/pull/638#issuecomment-1115719496
@inomp3042 , is there a JIRA for the pull request? If not, could you create one? I am about to merge this but the JIRA number is missing.
--
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
[GitHub] [ratis] inomp3042 commented on pull request #638: Add follower information for leader to know follower when follower is slow
Posted by GitBox <gi...@apache.org>.
inomp3042 commented on PR #638:
URL: https://github.com/apache/ratis/pull/638#issuecomment-1115742593
No, let me create a new one for this PR.
--
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