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 2020/11/19 06:26:46 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #289: RATIS-1161. Updating commit index of follower does not require term check
szetszwo opened a new pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
See https://issues.apache.org/jira/browse/RATIS-1161
----------------------------------------------------------------
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 #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527728935
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
Sure, it is a good idea to also check it at follower side.
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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] lokeshj1703 commented on pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-734218474
@szetszwo Thanks for the contribution! @GlenGeng Thanks for the reviews! I have committed the PR to master branch.
----------------------------------------------------------------
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 #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527603796
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
Thanks a lot for the hint! It would take me a long time to figure it out.
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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] lokeshj1703 commented on a change in pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527668633
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
@szetszwo Minor comment: instead of sending a truncated leaderCommit, should we use minimum of leader commit and entry size in appendEntriesAsync itself?
private CompletableFuture<AppendEntriesReplyProto> appendEntriesAsync(
RaftPeerId leaderId, long leaderTerm, TermIndex previous, long leaderCommit, long callId, boolean initializing,
List<CommitInfoProto> commitInfos, LogEntryProto... entries) throws IOException {
...
long commitIndex = min(leaderCommit, previous.getIndex() + entries.length);
state.updateCommitIndex(commitIndex, currentTerm, false);
...
----------------------------------------------------------------
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] lokeshj1703 edited a comment on pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
lokeshj1703 edited a comment on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-734218474
@szetszwo Thanks for the contribution! @runzhiwang @GlenGeng Thanks for the reviews! I have committed the PR to master branch.
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-734048255
@lokeshj1703 , could you take a look the latest change? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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] GlenGeng commented on a change in pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527520976
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
Thanks for the quick fix !
There is a subtle thing here: it should be min of leaderCommit and index of the last new appended entry from this AppendEntriesRequest.
<img width="389" alt="image" src="https://user-images.githubusercontent.com/8412672/99776540-f5202c00-2b4b-11eb-9bf7-a9469961dbe9.png">
Here is the fault scenario:
Given the format of each log entry is <term, index>,
follower has raft log as
``
<1,1>, <1,2>, <1,3>, <1,4>, <1,5>, <1,6>,
``
Its flush index is 6, commit index is 3 (which is get from leader of term 1)
leader has raft log as
``
<1,1>, <1,2>, <1,3>, <2,4>, <2,5>
``
Say, its commit index is 4
leader send an empty heartbeat with <previousTerm, previousIndex> as <1,3>, which can pass consistency check.
In this case, follower will advance its commit index to 4, but actually `<1,4>` is not a new added log entry.
----------------------------------------------------------------
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] GlenGeng commented on a change in pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527525039
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
<img width="646" alt="截屏2020-11-20 下午4 32 16" src="https://user-images.githubusercontent.com/8412672/99777973-0702ce80-2b4e-11eb-9505-466c122a09dc.png">
https://github.com/logcabin/logcabin/blob/master/Server/RaftConsensus.cc
This is how the author of raft handle this issue: he shrinks the commit index that it sends to the followers.
For above heartbeat case, since no entries is packed into the append entries request, the commit index received by the follower is just preLogIndex.
----------------------------------------------------------------
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] GlenGeng commented on a change in pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#discussion_r527520976
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java
##########
@@ -123,15 +123,20 @@ public boolean isOpened() {
* Update the last committed index.
* @param majorityIndex the index that has achieved majority.
* @param currentTerm the current term.
+ * @param isLeader Is this server the leader?
* @return true if update is applied; otherwise, return false, i.e. no update required.
*/
- public boolean updateLastCommitted(long majorityIndex, long currentTerm) {
+ public boolean updateLastCommitted(long majorityIndex, long currentTerm, boolean isLeader) {
try(AutoCloseableLock writeLock = writeLock()) {
final long oldCommittedIndex = getLastCommittedIndex();
final long newCommitIndex = Math.min(majorityIndex, getFlushIndex());
if (oldCommittedIndex < newCommitIndex) {
- // Only update last committed index for current term. See §5.4.2 in
- // paper for details.
+ if (!isLeader) {
Review comment:
Thanks for the quick fix !
There is a subtle thing here: it should be min of leaderCommit and index of the last new appended entry from this AppendEntriesRequest.
<img width="403" alt="截屏2020-11-20 下午4 29 09" src="https://user-images.githubusercontent.com/8412672/99777644-93f95800-2b4d-11eb-96bc-dabf3ac0eb48.png">
Here is the fault scenario:
Given the format of each log entry is <term, index>,
follower has raft log as
``
<1,1>, <1,2>, <1,3>, <1,4>, <1,5>, <1,6>,
``
Its flush index is 6, commit index is 3 (which is get from leader of term 1)
leader has raft log as
``
<1,1>, <1,2>, <1,3>, <2,4>, <2,5>
``
Say, its commit index is 4
leader send an empty heartbeat with <previousTerm, previousIndex> as <1,3>, which can pass consistency check.
In this case, follower will advance its commit index to 4, but actually `<1,4>` is not a new added log entry.
----------------------------------------------------------------
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 edited a comment on pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-730857056
@szetszwo Maybe this failed ut testWithLoad related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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] lokeshj1703 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
lokeshj1703 closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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 #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-730857056
@szetszwo Maybe this failed ut related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289#issuecomment-730898082
@runzhiwang , thanks for checking it. I also suspect the test failures are related to the change. Let's hold on this and revisit the new logic.
----------------------------------------------------------------
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 closed pull request #289: RATIS-1161. Updating commit index of follower does not require term check
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #289:
URL: https://github.com/apache/incubator-ratis/pull/289
----------------------------------------------------------------
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