[server] fix KV snapshot restoring fallback - #3790
Conversation
|
PTAL @fresh-borzoni @wuchong |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@zuston Thank you, a good catch 👍 LGTM overall, couple of comments
| * Returns whether the throwable or any of its causes indicates that snapshot data no longer | ||
| * exists in remote storage. | ||
| */ | ||
| public static boolean isSnapshotDataNotExists(Throwable throwable) { |
There was a problem hiding this comment.
This deletes the znode and the files, so a wrong true here is unrecoverable. A standby during failover also says File does not exist while the file is fine, should we confirm with fileSystem.exists() before treating the snapshot as broken?
And should we match on FileNotFoundException / NoSuchFileException too? Otherwise S3, OSS and GCS each need another string added here later.
| } | ||
|
|
||
| if (!isRegistered) { | ||
| if (ownsSharedFiles) { |
There was a problem hiding this comment.
discard() no longer touches the shared files, which is right, but it's not obvious from here who does.
What happens to the ones this snapshot alone referenced once its znode is gone?
| } | ||
|
|
||
| public void registerKvFileHandles(SharedKvFileRegistry registry, long snapshotID) { | ||
| checkState( |
There was a problem hiding this comment.
Any reason to drop the checkState completely? A double registration goes silent now.
| // Simulate snapshot corruption by deleting only one private file while its metadata and | ||
| // shared files remain intact. | ||
| assertThat(snapshot2.getKvSnapshotHandle().getPrivateFileHandles()).isNotEmpty(); | ||
| snapshot2.getKvSnapshotHandle().getPrivateFileHandles().get(0).getKvFileHandle().discard(); |
There was a problem hiding this comment.
Deleting a local file gives No such file or directory, so the Hadoop message from the issue isn't exercised on the replica path at all.
Could we add a small unit test on isSnapshotDataNotExists with a nested File does not exist chain?
| List<Long> attemptedSnapshotIds = new ArrayList<>(); | ||
| testKvSnapshotContext = | ||
| new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore); | ||
| new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore) { |
There was a problem hiding this comment.
The round-trip is the important bit as production always hands out a snapshot deserialized from ZK, and the base context handing out the live object is what hid this.
Shall we move it into TestSnapshotContext instead of an anonymous subclass?
2 participants
Purpose
Fix KV tablet recovery when the latest remote snapshot is broken or missing. The recovery process now correctly detects missing snapshot files reported by different filesystem implementations, removes the broken snapshot metadata, and safely falls back to the previous available snapshot.
Close #3740
Brief change log
File does not existerror message.latest snapshot → previous snapshotfallback flow.Tests
API and Format
Documentation