CRZbulabula commented on code in PR #17935:
URL: https://github.com/apache/iotdb/pull/17935#discussion_r3410924154
##########
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:
Fixed in 1c96b5bd96. `ApplicationStateMachineProxy.loadSnapshot` now throws
an `IOException` when `applicationStateMachine.loadSnapshot(...)` returns
`false`, so it no longer advances `lastAppliedTermIndex` and instead fails
`initialize`/`reinitialize` (Ratis treats the snapshot install as failed rather
than applied). `reinitialize()` now declares `throws IOException`, which the
Ratis `StateMachine` interface already permits.
##########
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:
Good catch — fixed in 7fe8e9c8e0. `ConfigPlanExecutor.loadSnapshot` now
returns a `boolean` (it returns `false` when the snapshot directory is missing
or when any snapshot processor throws), and
`ConfigRegionStateMachine.loadSnapshot` propagates that result instead of
unconditionally returning `true`. The boolean contract is now honored for
ConfigRegion 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]
