IGNITE-25502 fail node on index greater than current majority#7694
IGNITE-25502 fail node on index greater than current majority#7694dant3 wants to merge 5 commits intoapache:mainfrom
Conversation
| LOG.error("FATAL ERROR: Can't truncate logs before appliedId={}, lastIndexKept={}", this.appliedId, | ||
| lastIndexKept); | ||
| return; | ||
| LOG.error("Raft log suffix conflict: cannot truncate entries that have been applied to the state machine. " |
There was a problem hiding this comment.
Let's keep log format as everywhere else. nodeId={}, appliedId={}, lastIndexKept={} should be at the end surrounded by [].
Ex.: The partition will be moved to error state [nodeId={}, appliedId={}, lastIndexKept={}].",
There was a problem hiding this comment.
I think that there should be no dot after ], but I saw people adding it in other places, which confuses me tbh
|
I'll take a look |
| * @return persisted applied index, or 0 if unknown. | ||
| */ | ||
| default long getPersistedAppliedIndex() { | ||
| return 0; |
There was a problem hiding this comment.
Shouldn't we implement it for all listeners, especially for MS/CMG?
There was a problem hiding this comment.
We should, the same situation is possible there, maybe worth a separate ticket though
There was a problem hiding this comment.
I believe that the real answer is "no, we should not". MG and CMG only use one data storage per FSM, which always guarantees that min(lastAppliedIndex) matches max(lastAppliedIndex), and the corresponding troubled behavior is impossible. This is a distribution-zone-exclusive problem.
There was a problem hiding this comment.
Yeah, thanks for clarification @ibessonov
| return false; | ||
| } | ||
|
|
||
| // Restore appliedId so that unsafeTruncateSuffix() can reject truncation of applied entries. |
There was a problem hiding this comment.
why do we need this entire block of code? please explain
There was a problem hiding this comment.
This block restores appliedId from the state machine's persisted applied index so the guard is effective immediately after restart, before any entries are re-applied.
After a node restart, logManager.appliedId is transient and resets to 0. The unsafeTruncateSuffix() guard checks lastIndexKept < appliedId.getIndex(), but with appliedId=0, it can never reject anything. This effectively leads to possible data corruption.
There was a problem hiding this comment.
Maybe this explanation is worth copying into the comment in the code.
modules/raft/src/test/java/org/apache/ignite/raft/jraft/storage/impl/LogManagerTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public long getPersistedAppliedIndex() { |
There was a problem hiding this comment.
By the way, I had a great discussion with @rpuch and it seems that we have already fixed the possibility of having gap between persisted and applied index (see https://issues.apache.org/jira/browse/IGNITE-28216), so there is a chance that all this PR could be unnecessary
There was a problem hiding this comment.
IGNITE-28216 fixes one of the known root causes: specific missed lastApplied() bumps in partition processing. This PR adds a detection mechanism: if truncation below applied index ever happens (from this bug or any future bug), we fail the node instead of silently corrupting data.
As a result these are complementary. IGNITE-28216 prevents the gap; our PR catches it if it ever occurs anyway, due to some other unknown bug or nuance -- a similar future bug would again cause silent corruption with no indication of something went wrong.
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/service/RaftGroupListener.java
Outdated
Show resolved
Hide resolved
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java
Outdated
Show resolved
Hide resolved
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java
Outdated
Show resolved
Hide resolved
| LOG.error("FATAL ERROR: Can't truncate logs before appliedId={}, lastIndexKept={}", this.appliedId, | ||
| lastIndexKept); | ||
| return; | ||
| LOG.error("Raft log suffix conflict: cannot truncate entries that have been applied to the state machine. " |
There was a problem hiding this comment.
I think that there should be no dot after ], but I saw people adding it in other places, which confuses me tbh
modules/raft/src/main/java/org/apache/ignite/raft/jraft/StateMachine.java
Outdated
Show resolved
Hide resolved
modules/raft/src/test/java/org/apache/ignite/raft/jraft/storage/impl/LogManagerTest.java
Outdated
Show resolved
Hide resolved
ibessonov
left a comment
ibessonov
left a comment
There was a problem hiding this comment.
My last comment is optional. Please consult with other review participant on the topic of resolving current conversations with them, I can merge the PR once it's done. Thank you!
4 participants
https://issues.apache.org/jira/browse/IGNITE-25502