You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2021/01/25 12:48:31 UTC

[GitHub] [incubator-ratis] runzhiwang opened a new pull request #402: RATIS-1295. Use a config to enable/disable pre vote

runzhiwang opened a new pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402


   ## What changes were proposed in this pull request?
   
   Use a config to enable/disable pre vote
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/RATIS-1295
   
   ## How was this patch tested?
   
   no need to add new ut
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia edited a comment on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767090932


   Question: shouldn't pre-vote is always a good optimization? Is there a situation that it is not suitable thus needs to be disabled?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564132856



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       If we use false, when we change to true ? I think default true maybe better. Most application do not need to benchmark 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767182095






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang merged pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767090932


   Question: shouldn't pre-vote is always a good optimization to accelerate leader election? Is there a situation that it is not suitable thus needs to be disabled?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767198861


   Overall LGTM.
   
   We can discuss what could be the default value.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767090932






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564132856



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       If we use false, when we change to true ? I think default true maybe better.

##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       If we use false, when we change to true ? I think default true maybe better. Most application do not need to benchmark it.

##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       @szetszwo What do you think ?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564132856



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       If we use false, when we change to true ? I think default true maybe better.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767274555


   @szetszwo @amaliujia Thanks for review. I have merged the 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia edited a comment on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767090932


   Question: shouldn't pre-vote is always a good optimization? Is there a situation that it is not suitable thus needs to be disabled?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564175211



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       I do not have a strong opinion.  It seems okay to set true as the default.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564132141



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       Nit: might be better to make this default value as `false` so downstream applications (e.g. Ozone) won't feel surprising before benchmarking this optimization.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang merged pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564184112



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       Sure we can start from `true` then




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564132141



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       Nit: might be better to make this default value as `false` so downstream applications (e.g. Ozone) won't feel surprising before benchmarking this optimization.

##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       Sure we can start from `true` then




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767182095


   @amaliujia Please see here.
   ![image](https://user-images.githubusercontent.com/51938049/105779415-4cf26d80-5fa9-11eb-843e-accbd613c5cf.png)
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564175211



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       I do not have a strong opinion.  It seems okay to set true as the default.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] amaliujia commented on pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#issuecomment-767197941


   Thanks for explanation! Indeed for larger system it is better to benchmark such changes before adopting 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #402: RATIS-1295. Use a config to enable/disable pre vote

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #402:
URL: https://github.com/apache/incubator-ratis/pull/402#discussion_r564141340



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -555,6 +555,16 @@ static TimeDuration leaderStepDownWaitTime(RaftProperties properties) {
     static void setLeaderStepDownWaitTime(RaftProperties properties, TimeDuration noLeaderTimeout) {
       setTimeDuration(properties::setTimeDuration, LEADER_STEP_DOWN_WAIT_TIME_KEY, noLeaderTimeout);
     }
+
+    String PRE_VOTE_KEY = PREFIX + ".pre-vote";
+    boolean PRE_VOTE_DEFAULT = true;

Review comment:
       @szetszwo What do you think ?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org