Caideyipi commented on PR #17935: URL: https://github.com/apache/iotdb/pull/17935#issuecomment-4703934828
Thanks for fixing the snapshot-load failure propagation. The main AddPeer control flow looks reasonable, and the new test covers the case where the target state machine reports load failure. I found two concerns: 1. In `IoTConsensusServerImpl.loadSnapshot()`, the code now calls `stateMachine.loadSnapshot()` for every `recvSnapshotDir` and treats any false result as overall failure. However, snapshot fragments are written via `FolderManager` selection, so a given snapshot may not exist under every receive root. DataRegion also configures `recvSnapshotDirs` from all local data dirs, and `SnapshotLoader` fails when the given snapshot root does not exist. This can make a successful transfer fail in normal multi-data-dir deployments. We should either load only from the actual root that contains the received snapshot, or ensure all files of one snapshot are always placed under one selected root. A regression test with multiple `recvSnapshotDirs` and real snapshot files would help cover this. 2. `ApplicationStateMachineProxy` now logs when `applicationStateMachine.loadSnapshot()` returns false, but it still updates `lastAppliedTermIndex` afterward. That means Ratis may continue as if the snapshot was installed even though the application state machine rejected it. Since `loadSnapshot` now has an explicit success/failure contract, this path should avoid advancing the applied index, or fail initialization/reinitialization, when the load fails. -- 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]
