You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "chungen0126 (via GitHub)" <gi...@apache.org> on 2024/08/04 06:14:43 UTC
[PR] RATIS-2134. `logMetadata` could append wrong `commitIndex` in the `metadataEntry` [ratis]
chungen0126 opened a new pull request, #1130:
URL: https://github.com/apache/ratis/pull/1130
## What changes were proposed in this pull request?
Now, RaftLog appends a metadata entry with the lastCommitIndex and checks whether the entry at the lastCommitIndex is neither a metadata entry nor an entry with a smaller commit index.
However, RaftLog doesn't always log metadata entries in order. Sometimes, it misses some commit indexes. For example:
STATEMACHINELOGENTRY(1, 1)
STATEMACHINELOGENTRY(1, 2)
METADATAENTRY(commitIndex: 1)
If a METADATAENTRY is missed and the lastCommitIndex is a metadata entry, RaftLog won't append it.
## What is the link to the Apache JIRA
[RATIS-2134](https://issues.apache.org/jira/projects/RATIS/issues/RATIS-2134)
## How was this patch tested?
CI:
https://github.com/chungen0126/ratis/actions/runs/10233933768
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704842708
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -960,7 +957,14 @@ private void updateCommit(LogEntryHeader[] entriesToCommit) {
}
hasConfiguration |= entry.getLogEntryBodyCase() == LogEntryBodyCase.CONFIGURATIONENTRY;
raftLog.getRaftLogMetrics().onLogEntryCommitted(entry);
+ if (entry.getLogEntryBodyCase() != LogEntryBodyCase.METADATAENTRY) {
+ lastCommitIndex = entry.getIndex();
+ }
}
+ if (logMetadataEnabled) {
Review Comment:
done
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704905200
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -230,6 +230,9 @@ private long appendMetadataImpl(long term, long newCommitIndex) {
final long nextIndex;
try(AutoCloseableLock writeLock = writeLock()) {
nextIndex = getNextIndex();
+ if (nextIndex - 1 != newCommitIndex) {
+ return INVALID_LOG_INDEX;
+ }
Review Comment:
> ... However, if the commitIndex was always slower than the nextIndex, it won't append the metadata entry until commitIndex catches up.
No, it should not. A metadata entry should be appended as soon as the commit index is increased. In a busy cluster, commitIndex will never catch up nextIndex since it always has new entries.
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704376160
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -230,6 +230,9 @@ private long appendMetadataImpl(long term, long newCommitIndex) {
final long nextIndex;
try(AutoCloseableLock writeLock = writeLock()) {
nextIndex = getNextIndex();
+ if (nextIndex - 1 != newCommitIndex) {
+ return INVALID_LOG_INDEX;
+ }
Review Comment:
Let's add more information to your example. Suppose`entriesToCommit` are
- STATEMACHINELOGENTRY(1, 1)
- STATEMACHINELOGENTRY(1, 2)
- METADATAENTRY(term: 1, index: 3, commitIndex: 1)
and
- `nextIndex` is 4.
Then, `newCommitIndex` is 2 and `nextIndex - 1 != newCommitIndex` is false. It won't append the metadata entry in this case. Why checking the if-condition here?
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -960,7 +957,14 @@ private void updateCommit(LogEntryHeader[] entriesToCommit) {
}
hasConfiguration |= entry.getLogEntryBodyCase() == LogEntryBodyCase.CONFIGURATIONENTRY;
raftLog.getRaftLogMetrics().onLogEntryCommitted(entry);
+ if (entry.getLogEntryBodyCase() != LogEntryBodyCase.METADATAENTRY) {
+ lastCommitIndex = entry.getIndex();
+ }
}
+ if (logMetadataEnabled) {
Review Comment:
It should check if `lastCommitIndex != RaftLog.INVALID_LOG_INDEX`, i.e.
```java
if (logMetadataEnabled && lastCommitIndex != RaftLog.INVALID_LOG_INDEX) {
logMetadata(lastCommitIndex);
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -242,25 +245,11 @@ private boolean shouldAppendMetadata(long newCommitIndex) {
// do not log the first conf entry
return false;
} else if (Optional.ofNullable(lastMetadataEntry.get())
- .filter(e -> e.getIndex() == newCommitIndex || e.getMetadataEntry().getCommitIndex() >= newCommitIndex)
+ .filter(e -> e.getMetadataEntry().getCommitIndex() >= newCommitIndex)
.isPresent()) {
- //log neither lastMetadataEntry, nor entries with a smaller commit index.
+ // do not log entries with a smaller commit index.
return false;
Review Comment:
Let's simplify the code as below:
```java
private boolean shouldAppendMetadata(long newCommitIndex) {
if (newCommitIndex <= 0) {
// do not log the first conf entry
return false;
}
final LogEntryProto last = lastMetadataEntry.get();
// do not log entries with a smaller commit index.
return newCommitIndex > last.getMetadataEntry().getCommitIndex();
}
```
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704857284
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -242,25 +245,11 @@ private boolean shouldAppendMetadata(long newCommitIndex) {
// do not log the first conf entry
return false;
} else if (Optional.ofNullable(lastMetadataEntry.get())
- .filter(e -> e.getIndex() == newCommitIndex || e.getMetadataEntry().getCommitIndex() >= newCommitIndex)
+ .filter(e -> e.getMetadataEntry().getCommitIndex() >= newCommitIndex)
.isPresent()) {
- //log neither lastMetadataEntry, nor entries with a smaller commit index.
+ // do not log entries with a smaller commit index.
return false;
Review Comment:
done
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #1130:
URL: https://github.com/apache/ratis/pull/1130#issuecomment-2270435667
Done. I also add a check to avoid NullPointerException as below as mentioned in https://github.com/apache/ratis/pull/1130#discussion_r1704381262:
```
private boolean shouldAppendMetadata(long newCommitIndex) {
if (newCommitIndex <= 0) {
// do not log the first conf entry
return false;
}
final LogEntryProto last = lastMetadataEntry.get();
// do not log entries with a smaller commit index.
if (last == null) {
return false;
}
return newCommitIndex > last.getMetadataEntry().getCommitIndex();
}
```
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704910453
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -230,6 +230,9 @@ private long appendMetadataImpl(long term, long newCommitIndex) {
final long nextIndex;
try(AutoCloseableLock writeLock = writeLock()) {
nextIndex = getNextIndex();
+ if (nextIndex - 1 != newCommitIndex) {
+ return INVALID_LOG_INDEX;
+ }
Review Comment:
Got it, updated.
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1130:
URL: https://github.com/apache/ratis/pull/1130
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #1130:
URL: https://github.com/apache/ratis/pull/1130#discussion_r1704855115
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/RaftLogBase.java:
##########
@@ -230,6 +230,9 @@ private long appendMetadataImpl(long term, long newCommitIndex) {
final long nextIndex;
try(AutoCloseableLock writeLock = writeLock()) {
nextIndex = getNextIndex();
+ if (nextIndex - 1 != newCommitIndex) {
+ return INVALID_LOG_INDEX;
+ }
Review Comment:
> Let's add more information to your example. Suppose`entriesToCommit` are
>
> * STATEMACHINELOGENTRY(1, 1)
> * STATEMACHINELOGENTRY(1, 2)
> * METADATAENTRY(term: 1, index: 3, commitIndex: 1)
>
> and
>
> * `nextIndex` is 4.
In your case, `entriesToCommit` will become:
- STATEMACHINELOGENTRY(1, 1)
- STATEMACHINELOGENTRY(1, 2)
- METADATAENTRY(term: 1, index: 3, commitIndex: 2)
> Then, `newCommitIndex` is 2 and `nextIndex - 1 != newCommitIndex` is false. It won't append the metadata entry in this case. Why checking the if-condition here?
The metadata entry (term: 1, index: 3, commitIndex: 1) will not be appended because the condition `nextIndex - 1 != newCommitIndex` is false. Specifically, nextIndex - 1 (2) is not equal to newCommitIndex (1).
Ideally, when there are consecutive commits, it will only log one metadata with last commit index. However, if the `commitIndex` was always slower than the nextIndex, it won't append the metadata entry until `commitIndex` catches up.
--
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
Re: [PR] RATIS-2134. `logMetadata` could miss appending the `metadataEntry` with the lastCommitIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1130:
URL: https://github.com/apache/ratis/pull/1130#issuecomment-2270416440
Could you also change `logMetadata` as below as mentioned in https://github.com/apache/ratis/pull/1130#pullrequestreview-2219395735 ?
```java
//LeaderStateImpl
private void logMetadata(long commitIndex) {
if (raftLog.appendMetadata(currentTerm, commitIndex) != RaftLog.INVALID_LOG_INDEX) {
notifySenders();
}
}
```
--
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