HDDS-13228. Take snapshot cache lock during the last iteration of tarball transfer. #8678
+223
−26
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
smengcl
reviewed
Jul 2, 2025
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
Outdated
Show resolved
Hide resolved
jojochuang
reviewed
Jul 8, 2025
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
Outdated
Show resolved
Hide resolved
jojochuang
reviewed
Jul 8, 2025
...n-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java
Show resolved
Hide resolved
...ne-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java
Outdated
Show resolved
Hide resolved
| LOG.warn("SnapshotId: '{}' does not exist in snapshot cache.", k); | ||
| } else { | ||
| try { | ||
| v.get().getMetadataManager().getStore().flushDB(); |
Contributor
There was a problem hiding this comment.
is this necessary?
https://javadoc.io/doc/org.rocksdb/rocksdbjni/6.8.1/org/rocksdb/Options.html#avoidFlushDuringShutdown()
By default RocksDB will flush all memtables on DB close if there are unpersisted data (i.e. with WAL disabled) T
Contributor
Author
There was a problem hiding this comment.
Without this, I didn't find the .sst file visible in the rocksdb directory which is why I added it because the WAL is enabled in our case.
jojochuang
approved these changes
Jul 10, 2025
Contributor
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
A few cosmetic issues but overall looks fine. Made a few suggestions we can incorporate them in later PRs.
| OM_DB_NAME + checkpointDirName; | ||
| } | ||
|
|
||
| public static String extractSnapshotIDFromCheckpointDirName(String snapshotPath) { |
Contributor
There was a problem hiding this comment.
this public static method does not have test coverage, is only used by OMDBCheckpointServletInodeBasedXfer, and does not use any internal variables or method inside OmSnapshotManager. It should be moved into OMDBCheckpointServletInodeBasedXfer as a private method.
Otherwise if it is intended to be used as is later, please add javadoc and test coverage.
| when(responseMock.getOutputStream()).thenReturn(servletOutputStream); | ||
| } | ||
|
|
||
| String getValueFromSnapshotDeleteTable(String key, String snapshotDB) { |
Contributor
There was a problem hiding this comment.
Suggested change
| String getValueFromSnapshotDeleteTable(String key, String snapshotDB) { | |
| private String getValueFromSnapshotDeleteTable(String key, String snapshotDB) { |
| return OMConfigKeys.OZONE_OM_DB_DIRS; | ||
| } | ||
|
|
||
| public static List<String> getAllColumnFamilies() { |
Contributor
There was a problem hiding this comment.
Suggested change
| public static List<String> getAllColumnFamilies() { | |
| /** | |
| * Returns a list of all column family names defined in the database. | |
| * | |
| * @return list of column family names | |
| */ | |
| public static List<String> getAllColumnFamilies() { |
Contributor
Author
|
Thanks @jojochuang for the review. |
jojochuang
pushed a commit
to jojochuang/ozone
that referenced
this pull request
Jul 31, 2025
jojochuang
added a commit
to jojochuang/ozone
that referenced
this pull request
Nov 19, 2025
…n of tarball transfer. (apache#8678)" This reverts commit c706c7a. Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Change-Id: I77e2f08e4cc6b3c698387e3330985580ae117b69
jojochuang
added a commit
to jojochuang/ozone
that referenced
this pull request
Nov 20, 2025
…n of tarball transfer. (apache#8678)" This reverts commit c706c7a. Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
jojochuang
added a commit
that referenced
this pull request
Nov 21, 2025
…n of tarball transfer. (#8678)" This reverts commit c706c7a. Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServletInodeBasedXfer.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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?
Take snapshot cache lock during the last iteration of tarball transfer
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13228
How was this patch tested?
Unit tests