You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2025/08/29 20:56:30 UTC
[PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
swamirishi opened a new pull request, #8982:
URL: https://github.com/apache/ozone/pull/8982
## What changes were proposed in this pull request?
Currently Snapshot Create incorrectly tracks sst files from AOS db instead of the checkpoint creation. If there is a background compaction that got committed right after creation of checkpoint this could fetch wrong results.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13628
## How was this patch tested?
Existing unit tests
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2347713092
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -632,16 +636,13 @@ private static List<LiveFileMetaData> getSnapshotSSTFileList(RDBStore store)
/**
* Creates and writes snapshot local properties to a YAML file with uncompacted SST file list.
Review Comment:
Add `@param snapshotManager`
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8982:
URL: https://github.com/apache/ozone/pull/8982#issuecomment-3318509865
@sadanand48 @smengcl I had to fix the way I am opening the rocksdb because of the change in logic of the snapshot cache where we wait for the entire transaction to get flushed instead of just the mere check of snapshot directory exists
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8982:
URL: https://github.com/apache/ozone/pull/8982#issuecomment-3325102730
Thank you @sadanand48 @smengcl and @SaketaChalamchala for reviewing the patch
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2347713092
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -632,16 +636,13 @@ private static List<LiveFileMetaData> getSnapshotSSTFileList(RDBStore store)
/**
* Creates and writes snapshot local properties to a YAML file with uncompacted SST file list.
Review Comment:
Add `@param snapshotManager` and `@param snapshotInfo`
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi merged PR #8982:
URL: https://github.com/apache/ozone/pull/8982
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2311889255
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java:
##########
@@ -95,55 +119,62 @@ private File writeToYaml(String snapshotName) throws IOException {
// Check YAML file exists
assertTrue(yamlFile.exists());
- return yamlFile;
+ return Pair.of(yamlFile, previousSnapshotId);
}
@Test
public void testWriteToYaml() throws IOException {
- File yamlFile = writeToYaml("snapshot1");
+ Pair<File, UUID> yamlFilePrevIdPair = writeToYaml("snapshot1");
+ File yamlFile = yamlFilePrevIdPair.getLeft();
+ UUID prevSnapId = yamlFilePrevIdPair.getRight();
// Read from YAML file
OmSnapshotLocalDataYaml snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile);
// Verify fields
- assertEquals(42, snapshotData.getVersion());
+ assertEquals(44, snapshotData.getVersion());
Review Comment:
this would be 44 since version is going to be auto incremented every time a new version is added
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #8982:
URL: https://github.com/apache/ozone/pull/8982#issuecomment-3252084758
This PR requires #8981
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2311588303
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java:
##########
@@ -95,55 +119,62 @@ private File writeToYaml(String snapshotName) throws IOException {
// Check YAML file exists
assertTrue(yamlFile.exists());
- return yamlFile;
+ return Pair.of(yamlFile, previousSnapshotId);
}
@Test
public void testWriteToYaml() throws IOException {
- File yamlFile = writeToYaml("snapshot1");
+ Pair<File, UUID> yamlFilePrevIdPair = writeToYaml("snapshot1");
+ File yamlFile = yamlFilePrevIdPair.getLeft();
+ UUID prevSnapId = yamlFilePrevIdPair.getRight();
// Read from YAML file
OmSnapshotLocalDataYaml snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile);
// Verify fields
- assertEquals(42, snapshotData.getVersion());
+ assertEquals(44, snapshotData.getVersion());
Review Comment:
The test expects version 44 but the setup code sets version to 42. This causes a test assertion failure - the expected version should be 42 to match the setup.
```suggestion
assertEquals(42, snapshotData.getVersion());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -256,4 +321,26 @@ public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) throw
return dataYaml;
}
+
+ private static class OmSnapshotLocalDataYamlProvider {
+ private static volatile Yaml instance = getInstance();
Review Comment:
This creates infinite recursion during class initialization. The static field should be initialized to null, not by calling getInstance().
```suggestion
private static volatile Yaml instance = null;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -602,31 +607,22 @@ private static void deleteKeysFromDelKeyTableInSnapshotScope(
* @param store AOS or snapshot DB for uncompacted or compacted snapshot respectively.
* @return a Map of (table, set of SST files corresponding to the table)
*/
- private static Map<String, Set<String>> getSnapshotSSTFileList(RDBStore store)
+ private static List<LiveFileMetaData> getSnapshotSSTFileList(RDBStore store)
throws IOException {
- Map<String, Set<String>> sstFileList = new HashMap<>();
- List<LiveFileMetaData> liveFileMetaDataList = store.getDb().getLiveFilesMetaData();
- liveFileMetaDataList.forEach(lfm -> {
- String cfName = StringUtils.bytes2String(lfm.columnFamilyName());
- if (COLUMN_FAMILIES_TO_TRACK_IN_SNAPSHOT.contains(cfName)) {
- sstFileList.computeIfAbsent(cfName, k -> new HashSet<>()).add(lfm.fileName());
- }
- });
- return sstFileList;
+ return store.getDb().getLiveFilesMetaData().stream()
+ .filter(lfm -> COLUMN_FAMILIES_TO_TRACK_IN_SNAPSHOT.contains(StringUtils.bytes2String(lfm.columnFamilyName())))
+ .collect(Collectors.toList());
}
/**
* Creates and writes snapshot local properties to a YAML file with uncompacted SST file list.
- * @param omMetadataManager the metadata manager
- * @param snapshotInfo The metadata of snapshot to be created
- * @param store The store used to get uncompacted SST file list from.
+ * @param snapshotStore snapshot metadata manager.
*/
- public static void createNewOmSnapshotLocalDataFile(
- OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo, RDBStore store)
- throws IOException {
- Path snapshotLocalDataPath = Paths.get(getSnapshotLocalPropertyYamlPath(omMetadataManager, snapshotInfo));
+ public static void createNewOmSnapshotLocalDataFile(RDBStore snapshotStore, SnapshotInfo snapshotInfo) throws IOException {
+ Path snapshotLocalDataPath = Paths.get(getSnapshotLocalPropertyYamlPath(snapshotStore.getDbLocation().toPath()));
Review Comment:
The method signature changed to accept snapshotStore.getDbLocation().toPath() but getSnapshotLocalPropertyYamlPath likely expects different parameters. This may cause a compilation error or runtime failure.
```suggestion
Path snapshotLocalDataPath = getSnapshotLocalPropertyYamlPath(snapshotStore.getDbLocation().toPath());
```
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2311889090
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java:
##########
@@ -256,4 +321,26 @@ public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) throw
return dataYaml;
}
+
+ private static class OmSnapshotLocalDataYamlProvider {
+ private static volatile Yaml instance = getInstance();
Review Comment:
not used anymore.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13628. LocalSnapshotMetadata create should track snapshot from checkpoint instead of active object store [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #8982:
URL: https://github.com/apache/ozone/pull/8982#discussion_r2349433885
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java:
##########
@@ -755,7 +755,8 @@ private Set<String> getFiles(Path path, int truncateLength,
}
String filename = String.valueOf(file.getFileName());
if (!filename.startsWith("fabricated") &&
- !filename.startsWith(OZONE_RATIS_SNAPSHOT_COMPLETE_FLAG_NAME)) {
+ !filename.startsWith(OZONE_RATIS_SNAPSHOT_COMPLETE_FLAG_NAME) &&
+ !(filename.equals("archive") && file.getParent().getFileName().toString().startsWith("om.db"))) {
Review Comment:
findbugs warning:
```
M D NP: Possible null pointer dereference in org.apache.hadoop.ozone.om.TestOMDbCheckpointServlet.getFiles(Path, int, Set) due to return value of called method Dereferenced at TestOMDbCheckpointServlet.java:[line 759]
```
https://github.com/swamirishi/ozone/actions/runs/17618881306/job/50059192799
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org