Copilot commented on code in PR #17935:
URL: https://github.com/apache/iotdb/pull/17935#discussion_r3410751939
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ratis/ApplicationStateMachineProxy.java:
##########
@@ -268,7 +268,9 @@ private void loadSnapshot(File latestSnapshotDir) {
}
// require the application statemachine to load the latest snapshot
- applicationStateMachine.loadSnapshot(latestSnapshotDir);
+ if (!applicationStateMachine.loadSnapshot(latestSnapshotDir)) {
+ logger.error("{}: failed to load snapshot from {}", this,
latestSnapshotDir);
+ }
TermIndex snapshotTermIndex = Utils.getTermIndexFromDir(latestSnapshotDir);
updateLastAppliedTermIndex(snapshotTermIndex.getTerm(),
snapshotTermIndex.getIndex());
Review Comment:
If `applicationStateMachine.loadSnapshot(...)` returns `false`, this method
still updates `lastAppliedTermIndex` from the snapshot dir. That makes the
proxy report the snapshot as applied even though the application state machine
did not load it successfully, which can leave the node in an inconsistent
state. Consider returning early (or otherwise preventing the term/index update)
when snapshot load fails.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/statemachine/ConfigRegionStateMachine.java:
##########
@@ -246,20 +246,22 @@ public boolean takeSnapshot(File snapshotDir) {
}
@Override
- public void loadSnapshot(final File latestSnapshotRootDir) {
+ public boolean loadSnapshot(final File latestSnapshotRootDir) {
try {
executor.loadSnapshot(latestSnapshotRootDir);
// We recompute the snapshot for pipe listener when loading snapshot
// to recover the newest snapshot in cache
PipeConfigNodeAgent.runtime()
.listener()
.tryListenToSnapshots(ConfigNodeSnapshotParser.getSnapshots());
+ return true;
} catch (final IOException e) {
if (PipeConfigNodeAgent.runtime().listener().isOpened()) {
LOGGER.warn(
ConfigNodeMessages.CONFIG_REGION_LISTENING_QUEUE_LISTEN_TO_SNAPSHOT_FAILED_WHEN_STARTUP,
e);
}
+ return false;
}
Review Comment:
`loadSnapshot` returns `true` whenever `executor.loadSnapshot(...)` returns
normally, but `ConfigPlanExecutor#loadSnapshot` is `void` and internally
swallows failures (e.g., missing snapshot dir or processor exceptions are
logged and it simply returns). This means
`ConfigRegionStateMachine.loadSnapshot` can incorrectly report success even
when the snapshot wasn't loaded, which defeats the purpose of the new boolean
contract for callers like AddPeer.
--
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]
