CRZbulabula commented on PR #17935: URL: https://github.com/apache/iotdb/pull/17935#issuecomment-4704366407
Thanks for the careful review, @Caideyipi! Both concerns are addressed: **1. Multi-data-dir false failure in `IoTConsensusServerImpl.loadSnapshot`** (1c96b5bd96) You're right. A DataRegion sets `recvSnapshotDirs` to all local data dirs (`DataRegionConsensusImpl`), and the `FolderManager` spreads snapshot fragments across them, so a given snapshot only materializes under the folders that actually received fragments — loading from a folder that never received this snapshot would (correctly) fail and turn a healthy transfer into a spurious failure. I went with the "load only from the roots that contain the received snapshot" option: - Skip receive folders whose snapshot root does not exist, load from the ones that do, and still report failure if a present snapshot fails to load or if **no** folder received the snapshot at all. - The actual complete load happens from whichever root holds the snapshot log, since `SnapshotLoader#createLinksFromSnapshotDirToDataDirWithLog` gathers the fragments across all data dirs and verifies the logged file count, so skipping the empty roots is safe. I also added the regression test you suggested — `AddPeerSnapshotLoadFailureTest#addRemotePeerSucceedsWhenSnapshotSpansSubsetOfRecvDirs` — which builds a two-node group with multiple `recvSnapshotDirs`, transfers a real snapshot that lands in only a subset of the target's receive folders, and asserts AddPeer still succeeds. I verified it fails against the previous load-from-every-folder behavior and passes with the fix. **2. `ApplicationStateMachineProxy` advancing the applied index on load failure** (1c96b5bd96) `loadSnapshot` now throws `IOException` when the application state machine returns `false`, so `lastAppliedTermIndex` is no longer advanced and `initialize`/`reinitialize` fail instead of reporting the snapshot as applied. `reinitialize()` now declares `throws IOException`, which the Ratis `StateMachine` interface already permits. Separately, Copilot flagged that `ConfigRegionStateMachine.loadSnapshot` always returned `true` because `ConfigPlanExecutor.loadSnapshot` was `void` and swallowed failures — fixed in 7fe8e9c8e0 by making it return a boolean and propagating it. All consensus tests pass (`AddPeerSnapshotLoadFailureTest`, `ReplicateTest`/`StabilityTest`, `RatisConsensusTest`/`RecoverReadTest`/`SnapshotTest`). -- 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]
