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 2021/10/12 06:55:26 UTC
[GitHub] [ratis] lokeshj1703 opened a new pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
lokeshj1703 opened a new pull request #518:
URL: https://github.com/apache/ratis/pull/518
## What changes were proposed in this pull request?
RATIS-1411 introduces an optimization which uses follower gap limitation to control the number of requests accepted by leader. In case the follower gap is larger the pending requests are not cleared until gap narrows.
This also impacts the watch requests though. Since watch requests already take into account majority and all commit, the follower gap limitation is not required for them.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1415
## How was this patch tested?
Existing 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on a change in pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #518:
URL: https://github.com/apache/ratis/pull/518#discussion_r733418554
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -434,13 +434,14 @@ public void onFollowerCommitIndex(FollowerInfo follower, long commitIndex) {
}
private void commitIndexChanged() {
- getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex).ifPresent(m -> {
- // Normally, leader commit index is always ahead of followers.
- // However, after a leader change, the new leader commit index may
- // be behind some followers in the beginning.
- watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
- watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
- watchRequests.update(ReplicationLevel.MAJORITY, m.max);
+ getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex)
+ .ifPresent(m -> {
+ // Normally, leader commit index is always ahead of followers.
+ // However, after a leader change, the new leader commit index may
+ // be behind some followers in the beginning.
+ watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
+ watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
+ watchRequests.update(ReplicationLevel.MAJORITY, m.max);
Review comment:
I thought this would be more readable. Usually with method chaining it is difficult to know where next call begins. But I have reverted the change in latest commit.
--
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] bshashikant commented on pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #518:
URL: https://github.com/apache/ratis/pull/518#issuecomment-942013097
@ChenSammi / @szetszwo , can you have a look as well?
--
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] lokeshj1703 commented on pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #518:
URL: https://github.com/apache/ratis/pull/518#issuecomment-948356846
Thanks @szetszwo! I have addressed the review comments.
--
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 #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #518:
URL: https://github.com/apache/ratis/pull/518
--
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] lokeshj1703 commented on a change in pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #518:
URL: https://github.com/apache/ratis/pull/518#discussion_r730577424
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -434,13 +437,16 @@ public void onFollowerCommitIndex(FollowerInfo follower, long commitIndex) {
}
private void commitIndexChanged() {
- getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex).ifPresent(m -> {
- // Normally, leader commit index is always ahead of followers.
- // However, after a leader change, the new leader commit index may
- // be behind some followers in the beginning.
- watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
- watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
- watchRequests.update(ReplicationLevel.MAJORITY, m.max);
+ getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex,
+ getFollowerMaxGapThreshold(
+ RaftServerConfigKeys.Write.FOLLOWER_GAP_RATIO_MAX_DEFAULT))
Review comment:
Thanks @szetszwo ! This looks better.
I have added a change on similar lines. Please take a look.
--
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 change in pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #518:
URL: https://github.com/apache/ratis/pull/518#discussion_r728587740
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -434,13 +437,16 @@ public void onFollowerCommitIndex(FollowerInfo follower, long commitIndex) {
}
private void commitIndexChanged() {
- getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex).ifPresent(m -> {
- // Normally, leader commit index is always ahead of followers.
- // However, after a leader change, the new leader commit index may
- // be behind some followers in the beginning.
- watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
- watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
- watchRequests.update(ReplicationLevel.MAJORITY, m.max);
+ getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex,
+ getFollowerMaxGapThreshold(
+ RaftServerConfigKeys.Write.FOLLOWER_GAP_RATIO_MAX_DEFAULT))
Review comment:
We should simply pass -1 instead of computing the fixed value every time.
--
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 change in pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #518:
URL: https://github.com/apache/ratis/pull/518#discussion_r733299251
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -434,13 +434,14 @@ public void onFollowerCommitIndex(FollowerInfo follower, long commitIndex) {
}
private void commitIndexChanged() {
- getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex).ifPresent(m -> {
- // Normally, leader commit index is always ahead of followers.
- // However, after a leader change, the new leader commit index may
- // be behind some followers in the beginning.
- watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
- watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
- watchRequests.update(ReplicationLevel.MAJORITY, m.max);
+ getMajorityMin(FollowerInfo::getCommitIndex, raftLog::getLastCommittedIndex)
+ .ifPresent(m -> {
+ // Normally, leader commit index is always ahead of followers.
+ // However, after a leader change, the new leader commit index may
+ // be behind some followers in the beginning.
+ watchRequests.update(ReplicationLevel.ALL_COMMITTED, m.min);
+ watchRequests.update(ReplicationLevel.MAJORITY_COMMITTED, m.majority);
+ watchRequests.update(ReplicationLevel.MAJORITY, m.max);
Review comment:
This is just formatting. Please revert it.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -734,11 +735,19 @@ static long getMax(long[] sorted) {
}
private void updateCommit() {
- getMajorityMin(FollowerInfo::getMatchIndex, raftLog::getFlushIndex)
- .ifPresent(m -> updateCommit(m.majority, m.min));
+ getMajorityMin(FollowerInfo::getMatchIndex, raftLog::getFlushIndex,
+ followerMaxGapThreshold)
+ .ifPresent(m -> updateCommit(m.majority, m.min));
}
- private Optional<MinMajorityMax> getMajorityMin(ToLongFunction<FollowerInfo> followerIndex, LongSupplier logIndex) {
+ private Optional<MinMajorityMax> getMajorityMin(
+ ToLongFunction<FollowerInfo> followerIndex, LongSupplier logIndex) {
Review comment:
In Ratis, we use 120 characters for the line width. Please don't reformat the long lines.
--
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] bshashikant commented on pull request #518: RATIS-1415. Avoid follower gap limitation for watch requests
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #518:
URL: https://github.com/apache/ratis/pull/518#issuecomment-942013097
@ChenSammi / @szetszwo , can you have a look as well?
--
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