You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "zuston (via GitHub)" <gi...@apache.org> on 2026/07/28 08:58:36 UTC
[PR] [server] fix KV snapshot restoring fallback [fluss]
zuston opened a new pull request, #3790:
URL: https://github.com/apache/fluss/pull/3790
<!--
*Thank you very much for contributing to Fluss - we are happy that you want to help us improve Fluss. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [GitHub issue](https://github.com/apache/fluss/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no issue.
- Name the pull request in the format "[component] Title of the pull request", where *[component]* should be replaced by the name of the component being changed. Typically, this corresponds to the component label assigned to the issue (e.g., [kv], [log], [client], [flink], [rust], [python], [c++], [elixir]). Skip *[component]* if you are unsure about which is the best component.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes.
- Each pull request should address only one issue, not mix up code from multiple issues.
- **Generative AI disclosure:** Indicate whether generative AI tools were used in authoring this PR. If yes, specify the tool below.
- [ ] No generative AI tools used
- [ ] Yes (please specify the tool below)
**(The sections below can be removed for hotfixes or typos)**
-->
<!--
Generated-by: [Tool Name and Version] following [the guidelines](https://github.com/apache/fluss/blob/main/AGENTS.md)
-->
### Purpose
<!-- Linking this pull request to the issue -->
Linked issue: close #xxx
<!-- What is the purpose of the change -->
### Brief change log
<!-- Please describe the changes made in this pull request and explain how they address the issue -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "fresh-borzoni (via GitHub)" <gi...@apache.org>.
fresh-borzoni commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3708937396
##########
fluss-server/src/main/java/org/apache/fluss/server/replica/Replica.java:
##########
@@ -854,7 +854,7 @@ private void downloadKvSnapshots(CompletedSnapshot completedSnapshot, Path kvTab
try {
kvSnapshotDataDownloader.transferAllDataToDirectory(downloadSpec, closeableRegistry);
} catch (Exception e) {
- if (e.getMessage().contains(CompletedSnapshot.SNAPSHOT_DATA_NOT_EXISTS_ERROR_MESSAGE)) {
+ if (CompletedSnapshot.isSnapshotDataNotExists(e)) {
Review Comment:
This still takes the one-arg overload, and it's the branch that removes the znode and discards the files.
I tried it with snapshot 0 fully intact on disk and the downloader failing once with `IOException("Fail to download kv snapshot.", new FileNotFoundException("File does not exist: <sst>"))`: `handleSnapshotBroken(0)` fires and the store goes empty. Checking the download spec's handles before that call keeps the snapshot, and `testBrokenSnapshotRecovery` stays green because the private file it deletes really is gone. Shall we?
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -647,19 +685,23 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
assertThat(kvReplica.getKvTablet()).isNotNull();
KvTablet kvTablet = kvReplica.getKvTablet();
- // verify that the data from snapshot1 is restored (snapshot2 was broken and cleaned up)
- // snapshot1 should contain: k1->3,c and k3->4,d
+ assertThat(attemptedSnapshotIds).containsExactly(2L, 1L);
+ assertThat(kvSnapshotStore.getLatestCompletedSnapshot(tableBucket).getSnapshotID())
+ .isEqualTo(1);
+ for (String sharedFilePath : sharedFilePathsUsedByBothSnapshots) {
+ FsPath path = new FsPath(sharedFilePath);
+ assertThat(path.getFileSystem().exists(path)).isTrue();
+ }
+
+ // Snapshot1 is restored after snapshot2 is discarded.
List<Tuple2<byte[], byte[]>> expectedKeyValues =
getKeyValuePairs(
genKvRecords(
Tuple2.of("k1", new Object[] {3, "c"}),
+ Tuple2.of("k2", new Object[] {2, "b"}),
Tuple2.of("k3", new Object[] {4, "d"})));
verifyGetKeyValues(kvTablet, expectedKeyValues);
- // Verify the core functionality: KvTablet successfully initialized despite broken snapshot
- // The key test is that the system can handle broken snapshots and recover correctly
-
- // Verify that we successfully simulated the broken snapshot condition
File metadataFile = new File(snapshot2.getMetadataFilePath().getPath());
assertThat(metadataFile.exists()).isFalse();
Review Comment:
This one can't fail. `_METADATA` is written by `CompletedSnapshotStore` on the coordinator and `TestingCompletedKvSnapshotCommitter` doesn't, so the file never exists here, even for a healthy snapshot. The store assertion above already covers the cleanup, drop this one?
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -46,22 +46,47 @@ public class KvSnapshotHandle {
private final long incrementalSize;
/**
- * Once the shared states are registered, it is the {@link SharedKvFileRegistry}'s
- * responsibility to cleanup those shared states. But in the cases where the state handle is
- * discarded before performing the registration, the handle should delete all the shared states
- * created by it.
- *
- * <p>This variable is not null iff the handles was registered.
+ * Whether {@link #discard()} should directly delete the shared files. This is true for newly
+ * created handles until registration and false for restored handles.
+ */
+ private boolean ownsSharedFiles;
+
+ /**
+ * The registry where shared files were registered, or null if registration has not happened.
*/
private transient SharedKvFileRegistry sharedKvFileRegistry;
- public KvSnapshotHandle(
+ KvSnapshotHandle(
List<KvFileHandleAndLocalPath> sharedFileHandles,
List<KvFileHandleAndLocalPath> privateFileHandles,
- long incrementalSize) {
+ long incrementalSize,
+ boolean ownsSharedFiles) {
this.sharedFileHandles = sharedFileHandles;
this.privateFileHandles = privateFileHandles;
this.incrementalSize = incrementalSize;
+ this.ownsSharedFiles = ownsSharedFiles;
+ }
+
+ /** Creates a handle that directly discards its shared files until they are registered. */
+ public static KvSnapshotHandle create(
+ List<KvFileHandleAndLocalPath> sharedFileHandles,
+ List<KvFileHandleAndLocalPath> privateFileHandles,
+ long incrementalSize) {
+ return new KvSnapshotHandle(sharedFileHandles, privateFileHandles, incrementalSize, true);
+ }
+
+ /**
+ * Restores a handle that only references already-persisted shared files.
+ *
+ * <p>A restored handle never directly deletes shared files because it cannot prove that no
+ * other snapshot references them. Shared files are reclaimed only through registry-based
Review Comment:
Nothing registers a discarded snapshot, so the shared files only it referenced are never reclaimed by the registry either, they stay until the table dir is dropped. Leaking beats deleting a file snapshot1 still needs, so the trade is right, I'd just rather the comment said that instead. Plus a TODO and an issue for the reference=0 GC you mentioned?
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #3790:
URL: https://github.com/apache/fluss/pull/3790#issuecomment-5102388217
PTAL @fresh-borzoni @wuchong
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3711045904
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -46,22 +46,47 @@ public class KvSnapshotHandle {
private final long incrementalSize;
/**
- * Once the shared states are registered, it is the {@link SharedKvFileRegistry}'s
- * responsibility to cleanup those shared states. But in the cases where the state handle is
- * discarded before performing the registration, the handle should delete all the shared states
- * created by it.
- *
- * <p>This variable is not null iff the handles was registered.
+ * Whether {@link #discard()} should directly delete the shared files. This is true for newly
+ * created handles until registration and false for restored handles.
+ */
+ private boolean ownsSharedFiles;
+
+ /**
+ * The registry where shared files were registered, or null if registration has not happened.
*/
private transient SharedKvFileRegistry sharedKvFileRegistry;
- public KvSnapshotHandle(
+ KvSnapshotHandle(
List<KvFileHandleAndLocalPath> sharedFileHandles,
List<KvFileHandleAndLocalPath> privateFileHandles,
- long incrementalSize) {
+ long incrementalSize,
+ boolean ownsSharedFiles) {
this.sharedFileHandles = sharedFileHandles;
this.privateFileHandles = privateFileHandles;
this.incrementalSize = incrementalSize;
+ this.ownsSharedFiles = ownsSharedFiles;
+ }
+
+ /** Creates a handle that directly discards its shared files until they are registered. */
+ public static KvSnapshotHandle create(
+ List<KvFileHandleAndLocalPath> sharedFileHandles,
+ List<KvFileHandleAndLocalPath> privateFileHandles,
+ long incrementalSize) {
+ return new KvSnapshotHandle(sharedFileHandles, privateFileHandles, incrementalSize, true);
+ }
+
+ /**
+ * Restores a handle that only references already-persisted shared files.
+ *
+ * <p>A restored handle never directly deletes shared files because it cannot prove that no
+ * other snapshot references them. Shared files are reclaimed only through registry-based
Review Comment:
this is tracked by https://github.com/apache/fluss/issues/3489
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3702358353
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/CompletedSnapshot.java:
##########
@@ -195,6 +197,24 @@ public static FsPath getMetadataFilePath(FsPath snapshotLocation) {
return new FsPath(snapshotLocation, SNAPSHOT_METADATA_FILE_NAME);
}
+ /**
+ * 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) {
Review 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?
good suggestion.
> And should we match on FileNotFoundException / NoSuchFileException too? Otherwise S3, OSS and GCS each need another string added here later.
The type checks cover structured exceptions, while Hadoop may wrap the remote failure as a generic IOException and preserve only the “File does not exist” message. Therefore, the message matching is still required as a compatibility fallback.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3711069014
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -647,19 +685,23 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
assertThat(kvReplica.getKvTablet()).isNotNull();
KvTablet kvTablet = kvReplica.getKvTablet();
- // verify that the data from snapshot1 is restored (snapshot2 was broken and cleaned up)
- // snapshot1 should contain: k1->3,c and k3->4,d
+ assertThat(attemptedSnapshotIds).containsExactly(2L, 1L);
+ assertThat(kvSnapshotStore.getLatestCompletedSnapshot(tableBucket).getSnapshotID())
+ .isEqualTo(1);
+ for (String sharedFilePath : sharedFilePathsUsedByBothSnapshots) {
+ FsPath path = new FsPath(sharedFilePath);
+ assertThat(path.getFileSystem().exists(path)).isTrue();
+ }
+
+ // Snapshot1 is restored after snapshot2 is discarded.
List<Tuple2<byte[], byte[]>> expectedKeyValues =
getKeyValuePairs(
genKvRecords(
Tuple2.of("k1", new Object[] {3, "c"}),
+ Tuple2.of("k2", new Object[] {2, "b"}),
Tuple2.of("k3", new Object[] {4, "d"})));
verifyGetKeyValues(kvTablet, expectedKeyValues);
- // Verify the core functionality: KvTablet successfully initialized despite broken snapshot
- // The key test is that the system can handle broken snapshots and recover correctly
-
- // Verify that we successfully simulated the broken snapshot condition
File metadataFile = new File(snapshot2.getMetadataFilePath().getPath());
assertThat(metadataFile.exists()).isFalse();
Review Comment:
nice catch. removed
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "fresh-borzoni (via GitHub)" <gi...@apache.org>.
fresh-borzoni merged PR #3790:
URL: https://github.com/apache/fluss/pull/3790
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3702506940
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -123,11 +133,8 @@ public void discard() {
}
public void registerKvFileHandles(SharedKvFileRegistry registry, long snapshotID) {
- checkState(
Review Comment:
undo this part
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3711055625
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -46,22 +46,47 @@ public class KvSnapshotHandle {
private final long incrementalSize;
/**
- * Once the shared states are registered, it is the {@link SharedKvFileRegistry}'s
- * responsibility to cleanup those shared states. But in the cases where the state handle is
- * discarded before performing the registration, the handle should delete all the shared states
- * created by it.
- *
- * <p>This variable is not null iff the handles was registered.
+ * Whether {@link #discard()} should directly delete the shared files. This is true for newly
+ * created handles until registration and false for restored handles.
+ */
+ private boolean ownsSharedFiles;
+
+ /**
+ * The registry where shared files were registered, or null if registration has not happened.
*/
private transient SharedKvFileRegistry sharedKvFileRegistry;
- public KvSnapshotHandle(
+ KvSnapshotHandle(
List<KvFileHandleAndLocalPath> sharedFileHandles,
List<KvFileHandleAndLocalPath> privateFileHandles,
- long incrementalSize) {
+ long incrementalSize,
+ boolean ownsSharedFiles) {
this.sharedFileHandles = sharedFileHandles;
this.privateFileHandles = privateFileHandles;
this.incrementalSize = incrementalSize;
+ this.ownsSharedFiles = ownsSharedFiles;
+ }
+
+ /** Creates a handle that directly discards its shared files until they are registered. */
+ public static KvSnapshotHandle create(
+ List<KvFileHandleAndLocalPath> sharedFileHandles,
+ List<KvFileHandleAndLocalPath> privateFileHandles,
+ long incrementalSize) {
+ return new KvSnapshotHandle(sharedFileHandles, privateFileHandles, incrementalSize, true);
+ }
+
+ /**
+ * Restores a handle that only references already-persisted shared files.
+ *
+ * <p>A restored handle never directly deletes shared files because it cannot prove that no
+ * other snapshot references them. Shared files are reclaimed only through registry-based
Review Comment:
I have updated the comment
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3702517912
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -618,13 +624,24 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
assertThat(kvSnapshotStore.getLatestCompletedSnapshot(tableBucket).getSnapshotID())
.isEqualTo(2);
- // now simulate the latest snapshot (snapshot2) being broken by
- // deleting its metadata files and unshared SST files
- // This simulates file corruption while ZK metadata remains intact
- snapshot2.getKvSnapshotHandle().discard();
+ Set<String> sharedFilePathsUsedByBothSnapshots =
+ snapshot1.getKvSnapshotHandle().getSharedKvFileHandles().stream()
+ .map(handle -> handle.getKvFileHandle().getFilePath())
+ .collect(Collectors.toSet());
+ sharedFilePathsUsedByBothSnapshots.retainAll(
+ snapshot2.getKvSnapshotHandle().getSharedKvFileHandles().stream()
+ .map(handle -> handle.getKvFileHandle().getFilePath())
+ .collect(Collectors.toSet()));
+ assertThat(sharedFilePathsUsedByBothSnapshots).isNotEmpty();
+ for (String sharedFilePath : sharedFilePathsUsedByBothSnapshots) {
+ FsPath path = new FsPath(sharedFilePath);
+ assertThat(path.getFileSystem().exists(path)).isTrue();
+ }
- // ZK metadata should still show snapshot2 as latest (file corruption hasn't been detected
- // yet)
+ // 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();
Review Comment:
added in the `CompletedSnapshotTest`
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3702498361
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -110,7 +120,7 @@ public void discard() {
LOG.warn("Could not properly discard misc file states.", e);
}
- if (!isRegistered) {
+ if (ownsSharedFiles) {
Review Comment:
this part is also missing in the current codebase. maybe we need introduce another GC mechanism to remove these reference=0 files.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3702526176
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -634,8 +651,31 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
// create a new replica with the same snapshot context
// During initialization, it will try to use snapshot2 but find it broken,
// then handle the broken snapshot and fall back to snapshot1
+ List<Long> attemptedSnapshotIds = new ArrayList<>();
testKvSnapshotContext =
- new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore);
+ new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore) {
Review Comment:
Moved the JSON round-trip into the base `TestSnapshotContext` . The anonymous provider now delegates to the base implementation and only records the attempted snapshot IDs for verifying the fallback order.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] fix KV snapshot restoring fallback [fluss]
Posted by "fresh-borzoni (via GitHub)" <gi...@apache.org>.
fresh-borzoni commented on code in PR #3790:
URL: https://github.com/apache/fluss/pull/3790#discussion_r3694207717
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/CompletedSnapshot.java:
##########
@@ -195,6 +197,24 @@ public static FsPath getMetadataFilePath(FsPath snapshotLocation) {
return new FsPath(snapshotLocation, SNAPSHOT_METADATA_FILE_NAME);
}
+ /**
+ * 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) {
Review 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.
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -110,7 +120,7 @@ public void discard() {
LOG.warn("Could not properly discard misc file states.", e);
}
- if (!isRegistered) {
+ if (ownsSharedFiles) {
Review 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?
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -618,13 +624,24 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
assertThat(kvSnapshotStore.getLatestCompletedSnapshot(tableBucket).getSnapshotID())
.isEqualTo(2);
- // now simulate the latest snapshot (snapshot2) being broken by
- // deleting its metadata files and unshared SST files
- // This simulates file corruption while ZK metadata remains intact
- snapshot2.getKvSnapshotHandle().discard();
+ Set<String> sharedFilePathsUsedByBothSnapshots =
+ snapshot1.getKvSnapshotHandle().getSharedKvFileHandles().stream()
+ .map(handle -> handle.getKvFileHandle().getFilePath())
+ .collect(Collectors.toSet());
+ sharedFilePathsUsedByBothSnapshots.retainAll(
+ snapshot2.getKvSnapshotHandle().getSharedKvFileHandles().stream()
+ .map(handle -> handle.getKvFileHandle().getFilePath())
+ .collect(Collectors.toSet()));
+ assertThat(sharedFilePathsUsedByBothSnapshots).isNotEmpty();
+ for (String sharedFilePath : sharedFilePathsUsedByBothSnapshots) {
+ FsPath path = new FsPath(sharedFilePath);
+ assertThat(path.getFileSystem().exists(path)).isTrue();
+ }
- // ZK metadata should still show snapshot2 as latest (file corruption hasn't been detected
- // yet)
+ // 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();
Review 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?
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/snapshot/KvSnapshotHandle.java:
##########
@@ -123,11 +133,8 @@ public void discard() {
}
public void registerKvFileHandles(SharedKvFileRegistry registry, long snapshotID) {
- checkState(
Review Comment:
Any reason to drop the checkState completely? A double registration goes silent now.
##########
fluss-server/src/test/java/org/apache/fluss/server/replica/ReplicaTest.java:
##########
@@ -634,8 +651,31 @@ void testBrokenSnapshotRecovery(@TempDir File snapshotKvTabletDir) throws Except
// create a new replica with the same snapshot context
// During initialization, it will try to use snapshot2 but find it broken,
// then handle the broken snapshot and fall back to snapshot1
+ List<Long> attemptedSnapshotIds = new ArrayList<>();
testKvSnapshotContext =
- new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore);
+ new TestSnapshotContext(snapshotKvTabletDir.getPath(), kvSnapshotStore) {
Review 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?
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org