HDDS-11914. Snapshot diff should not filter SST Files based by reading SST file reader #7563
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g SST file reader Change-Id: If657ea759afd90e87fc4e8e975546235d559401e
hemantk-12
reviewed
Dec 11, 2024
...-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDiffUtils.java
Outdated
Show resolved
Hide resolved
...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
Outdated
Show resolved
Hide resolved
hemantk-12
reviewed
Dec 11, 2024
...ds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java
Outdated
Show resolved
Hide resolved
...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
Outdated
Show resolved
Hide resolved
...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
Outdated
Show resolved
Hide resolved
hemantk-12
reviewed
Dec 11, 2024
...ds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java
Outdated
Show resolved
Hide resolved
Change-Id: Ic5518fc7631fbac7caac8ffb5b743c157168524f
hemantk-12
reviewed
Dec 12, 2024
...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
Outdated
Show resolved
Hide resolved
Change-Id: I189e26e59142ef4f6d1f035e6663bcc1639a59f4
hemantk-12
reviewed
Dec 12, 2024
...-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDiffUtils.java
Show resolved
Hide resolved
Change-Id: I6160cb11bd5d942bf9990e0ab1ca6014b2250bf1
Change-Id: I9a87f513b41982e3695b13f3ad6c4a17e2230579
hemantk-12
reviewed
Dec 12, 2024
Contributor
hemantk-12
left a comment
hemantk-12
left a comment
There was a problem hiding this comment.
Thanks @swamirishi for the patch.
LGTM.
prashantpogde
approved these changes
Dec 12, 2024
Contributor
prashantpogde
left a comment
prashantpogde
left a comment
There was a problem hiding this comment.
Changes look good to me. Thank you for making tehse changes @swamirishi
Change-Id: Ib0dde58b036f4061327806c8a2f8a460464bf224 # Conflicts: # hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
Change-Id: Iaf7b1bf0b1c986a8956e3fdfa0598dff4f56766f
Change-Id: I9f37dc42e98fbf9995a659ddabbd14debe940d78 # Conflicts: # hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java # hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDiffUtils.java
Change-Id: I9007e70744a6fa1f74f5d3411d469e20cb46b093
hemantk-12
reviewed
Dec 13, 2024
| RocksDiffUtils.filterRelevantSstFiles(inputFiles, tablePrefixes, fromDB); | ||
| deltaFiles.addAll(inputFiles); | ||
| } | ||
| addToObjectIdMap(fsTable, tsTable, deltaFiles, |
Contributor
There was a problem hiding this comment.
nit: can you please log deltaFiles at DEBUG level?
hemantk-12
approved these changes
Dec 13, 2024
Contributor
hemantk-12
left a comment
hemantk-12
left a comment
There was a problem hiding this comment.
Thanks @swamirishi for the fix.
LGTM.
Contributor
Author
|
Thanks @hemantk-12 @prashantpogde for reviewing the patch |
adoroszlai
reviewed
Dec 13, 2024
Comment on lines
-828
to
+839
| public synchronized Optional<List<String>> getSSTDiffListWithFullPath( | ||
| DifferSnapshotInfo src, | ||
| DifferSnapshotInfo dest, | ||
| String sstFilesDirForSnapDiffJob | ||
| ) throws IOException { | ||
| public synchronized Optional<List<String>> getSSTDiffListWithFullPath(DifferSnapshotInfo src, | ||
| DifferSnapshotInfo dest, | ||
| String sstFilesDirForSnapDiffJob) { |
Contributor
There was a problem hiding this comment.
nit: Indenting parameters like this triggers unnecessary change whenever visibility / return type / method name is updated.
| import org.apache.ratis.util.TimeDuration; | ||
| import jakarta.annotation.Nonnull; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.Assertions; |
Contributor
There was a problem hiding this comment.
nit: please use static import for assertions and mocks (see HDDS-9961).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Snapshot diff should not filter SST Files based by reading SST file reader and instead should rely on rocksdb's liveMetadataFile which also includes tombstone entries.
When creating dag compaction log as well, key ranges should also look at liveMetadataFile.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11914
How was this patch tested?
Existing Unit tests and adding more