ibessonov commented on code in PR #7694:
URL: https://github.com/apache/ignite-3/pull/7694#discussion_r3015539583
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/service/RaftGroupListener.java:
##########
@@ -96,4 +96,14 @@ default void onConfigurationCommitted(
* Invoked once after a raft node has been shut down.
*/
void onShutdown();
+
+ /**
+ * Returns the last applied index persisted by the state machine.
+ * Called during {@code NodeImpl.init()} to prevent truncation of
already-applied log entries.
Review Comment:
Maybe we should rephrase the `{@code NodeImpl.init()}`. It's not `{@link
...}`, which implies that the dependency is missing , because it's circular I
guess. Having circular references it not something that we want
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -1066,6 +1066,19 @@ public boolean init(final NodeOptions opts) {
return false;
}
+ // Restore appliedId so that unsafeTruncateSuffix() can reject
truncation of applied entries.
+ long persistedApplied =
this.options.getFsm().getPersistedAppliedIndex();
+ if (persistedApplied > 0) {
+ long term = this.logManager.getTerm(persistedApplied);
+ if (term > 0) {
+ // Term is 0 when the index is outside the log (covered by a
snapshot) — skip in that case.
+ this.logManager.setAppliedId(new LogId(persistedApplied,
term));
+ } else {
+ LOG.warn("Node {} persisted applied index {} is not in the
raft log, expecting snapshot to cover it.",
Review Comment:
Are there words missing in this sentence? I can't understand its grammatical
structure. Please check it.
I would also ask you to follow logging guidelines
(https://cwiki.apache.org/confluence/display/IGNITE/Java+Logging+Guidelines)
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -1066,6 +1066,19 @@ public boolean init(final NodeOptions opts) {
return false;
}
+ // Restore appliedId so that unsafeTruncateSuffix() can reject
truncation of applied entries.
+ long persistedApplied =
this.options.getFsm().getPersistedAppliedIndex();
+ if (persistedApplied > 0) {
+ long term = this.logManager.getTerm(persistedApplied);
+ if (term > 0) {
+ // Term is 0 when the index is outside the log (covered by a
snapshot) — skip in that case.
Review Comment:
Shouldn't this line of comment be in `else` branch? It looks out of place
now, at least for me
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/StateMachine.java:
##########
@@ -133,4 +133,14 @@ default void
onRawConfigurationCommitted(ConfigurationEntry conf, long lastAppli
* @param ctx context of leader change
*/
void onStartFollowing(final LeaderChangeContext ctx);
+
+ /**
+ * Returns the last applied index persisted by the state machine.
+ * Called during {@code NodeImpl.init()} to prevent truncation of
already-applied log entries.
Review Comment:
Please use `{@link ... }`
##########
modules/raft/src/test/java/org/apache/ignite/raft/jraft/storage/impl/LogManagerTest.java:
##########
@@ -476,4 +482,46 @@ public void testLastLogIndexWhenShutdown() throws
Exception {
Exception e = assertThrows(IllegalStateException.class, () ->
this.logManager.getLastLogIndex(true));
assertEquals("Node is shutting down", e.getMessage());
}
+
+ /** Suffix truncation below appliedId must report error and abort the
append (IGNITE-25502). */
Review Comment:
I agree, we don't need to link anything, git history is enough to restore
the context
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/storage/impl/LogManagerImpl.java:
##########
@@ -1049,11 +1049,25 @@ private boolean reset(final long nextLogIndex) {
}
}
- private void unsafeTruncateSuffix(final long lastIndexKept, final Lock
lock) {
+ /**
+ * Truncates log entries after {@code lastIndexKept}.
+ *
+ * @return {@code true} on success, {@code false} if truncation would
discard applied entries (node moves to error state).
+ */
+ private boolean unsafeTruncateSuffix(final long lastIndexKept, final Lock
lock) {
if (lastIndexKept < this.appliedId.getIndex()) {
- 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. "
Review Comment:
I think that there should be no dot after `]`, but I saw people adding it in
other places, which confuses me tbh
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
