HDDS-13006. Use yaml files to host Ozone snapshot local properties #8555

smengcl · 2025-06-03T17:54:42Z

What changes were proposed in this pull request?

Use one .yaml file for each Ozone snapshot DB to store local properties for each Ozone snapshop DB checkpoint. e.g. isSstFiltered
Tweaked SstFilteringService to use the new implementation, while keeping the old logic around until we have upgrade logic written.

Written with aid with Claude 3.7 Sonnet.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13006

How was this patch tested?

Added unit tests in TestOmSnapshotLocalPropertyYamlImpl.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java

swamirishi · 2025-06-03T23:21:11Z

Thanks for sharing the base interface for the snapshot property. I have left some comments inline

jojochuang · 2025-06-04T17:25:17Z

Please take a look at Gemini's review report too: jojochuang#569 (review)

jojochuang

Not sure what I'm looking at... are you missing a few source code files? @smengcl

smengcl · 2025-06-04T18:46:54Z

Not sure what I'm looking at... are you missing a few source code files? @smengcl

Yup this is intentional. This is a (real) draft. As discussed offline with @swamirishi and @SaketaChalamchala earlier, the first step would be to determine the interface so that @SaketaChalamchala can use it.

Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked)

…ocalPropertyYamlImpl`. Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked)

smengcl · 2025-06-08T06:55:24Z

This one is ready for review now. @swamirishi @jojochuang

swamirishi

Thanks for working on the patch @smengcl IMO we can make the changes in the interface to tightly coupled with the schema of SnapshotMetaData which is not going to change much

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java

swamirishi · 2025-06-09T19:51:57Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

+    // First try to read the flag from YAML file
+    String yamlPath = OmSnapshotManager.getSnapshotLocalPropertyYamlPath(ozoneConfiguration, snapshotInfo);
+
+    try (OmSnapshotLocalProperty localProperties = new OmSnapshotLocalPropertyYamlImpl(yamlPath)) {


There could be a potential race condition if multiple threads parallely try to open. It would be better if this comes from snapshotCache or make the cache abstract (ReferenceCountedCache) enough to load any resource and have one implementation for loading OmSnapshot and one reference counted cache implementation for OmSnapshotLocalProperty

OmSnapshotLocalProperty can also take a write lock on the snapshotId to prevent SNAPSHOT_PROPERTY_LOCK from getting updated by multiple threads. This could be simpler.

Agree. If we want it to be race-free, an easy thought would be to guard r/w access behind SnapshotCache r/w lock. But one thing I don't like is that it implies we are opening the snapshot DB just for reading the yaml metadata (assuming no major refactoring of the SnapshotCache).

Let me see if flock would work..

yeah lock would be simpler. We can have duplicate objects in that case

We would also need to take a bootstrap lock here

BootstrapStateHandler.java

We might not need bootstrap handler lock. Since we would be taking a lock on all the background services which would be updating the metadata file and no ratis transaction excepting for create snapshot is going to update this metadata file.

smengcl · 2025-06-09T21:58:45Z

TODO:

Use POJO
Add checksum field
Add R/W lock

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java

…ests.

…ogic in `SstFilteringService`

…pshotLocalDataYaml`

…, not selective ones.

jojochuang · 2025-06-16T20:50:26Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java

+    // Dump yaml data into a string to compute its checksum
+    String snapshotDataYamlStr = yaml.dump(this);
+
+    this.checksum = getChecksum(snapshotDataYamlStr);


if the YARML library changes output format, or if we add additional fields to it, it can break on upgrade.
I'm not sure how to handle it better though.

Good point. Checksum should be calculated before it is deserialized into a OmSnapshotLocalData* somehow. I didn't have an elegant way to handle this on top of my mind.

In other words, this behavior might be broken for ContainerData (ContainerDataYaml) as well. Need to check.

As discussed offline we can do this in a separate jira that handles the upgrade. Basically I have an idea that we can use the version field together with the SnapshotLocalDataRepresenter that I removed in 73c1702 (#8555) to make sure that checksum calculation is not broken with new fields being added in later yaml versions.

jojochuang · 2025-06-16T21:57:23Z

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java

+   * @param yamlFile The file to write to
+   * @throws IOException If there's an error writing to the file
+   */
+  public void writeToYaml(File yamlFile) throws IOException {


is there atomicity or consistency requirements using this class?

Yes. We might use existing SNAPSHOT_DB_LOCK or a new SNAPSHOT_LOCAL_DATA_LOCK lock to lock it during read/write but that would be up to the caller to implement that. It is out of the scope of this PR.

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java

smengcl · 2025-06-24T16:46:57Z

The implementation is done in the scope of the jira. Checksum-versioning would be added in a follow-up jira. Pls take another look at this. Thx. @SaketaChalamchala @jojochuang

jojochuang

LGTM. A few comments can be addressed later. Merging it now.

jojochuang · 2025-07-10T00:52:18Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java

+   * Sets the last compaction time, in epoch milliseconds.
+   * @param lastCompactionTime Timestamp of the last compaction
+   */
+  public void setLastCompactionTime(Long lastCompactionTime) {


Minor issue: The setLastCompactionTime method accepts a Long object, but the lastCompactionTime field is a primitive long. If a null value is passed to this method, it will result in a NullPointerException when unboxing. To prevent this, the parameter type should be changed to the primitive long.

jojochuang · 2025-07-10T00:57:16Z

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java

+    PropertyUtils propertyUtils = new PropertyUtils();
+    propertyUtils.setBeanAccess(BeanAccess.FIELD);
+    propertyUtils.setAllowReadOnlyProperties(true);
+
+    DumperOptions options = new DumperOptions();
+    Representer representer = new Representer(options);
+    representer.setPropertyUtils(propertyUtils);
+
+    SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor();
+
+    Yaml yaml = new Yaml(snapshotDataConstructor, representer);


Nit: It seems this can be simplified.

Suggested change

PropertyUtils propertyUtils = new PropertyUtils();

propertyUtils.setBeanAccess(BeanAccess.FIELD);

propertyUtils.setAllowReadOnlyProperties(true);

DumperOptions options = new DumperOptions();

Representer representer = new Representer(options);

representer.setPropertyUtils(propertyUtils);

SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor();

Yaml yaml = new Yaml(snapshotDataConstructor, representer);

final Yaml yaml = getYamlForSnapshotLocalData();

However getYamlForSnapshotLocalData() also invokes "representer.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG);" which is not repeated here.

jojochuang · 2025-07-10T00:59:13Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java

+
+  /**
+   * Adds an entry to the compacted SST file list.
+   * @param ver Version number (TODO: to be clarified)


jojochuang · 2025-07-10T01:23:56Z

Error:  Used undeclared dependencies found:
Error:     org.yaml:snakeyaml:jar:2.0:compile
Error:  Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.8.1:analyze-only (analyze) on project ozone-manager: Dependency problems found -> [Help 1]

Change-Id: I6f258b5f8eb08affd4438f640471d927a6599c3d

…pache#8555) Co-authored-by: Wei-Chiu Chuang <[email protected]> Generated-by: Claude 3.7 Sonnet

HDDS-13006. Use yaml files to host Ozone snapshot local properties

22c634f

smengcl added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jun 3, 2025

swamirishi reviewed Jun 3, 2025

View reviewed changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java Outdated Show resolved Hide resolved

jojochuang reviewed Jun 4, 2025

View reviewed changes

smengcl added 7 commits June 7, 2025 23:02

Add implementation OmSnapshotLocalPropertyYamlImpl.

03bc93d

Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked)

Modify SstFilteringService to use OmSnapshotLocalPropertyYamlImpl.

f7eb655

Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked)

Add unit tests TestOmSnapshotLocalPropertyYamlImpl for `OmSnapshotL…

b9554a8

…ocalPropertyYamlImpl`. Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked)

Tweak Impl

56afd4e

Tweak doc

08a90ff

Make getProperties() return immutable map.

f5a46a8

Cleanup

b86888b

swamirishi requested changes Jun 9, 2025

View reviewed changes

SaketaChalamchala reviewed Jun 10, 2025

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Show resolved Hide resolved

smengcl added 3 commits June 10, 2025 22:47

Remove OmSnapshotLocalProperty, OmSnapshotLocalPropertyImpl and t…

6dfd198

…ests.

Add OmSnapshotLocalData and its YAML impl; add UTs; removed added l…

545ceb5

…ogic in `SstFilteringService`

Polish

164652d

swamirishi mentioned this pull request Jun 12, 2025

HDDS-13244. TestSnapshotDeletingServiceIntegrationTest should close snapshots after deleting them #8611

Merged

smengcl added 2 commits June 12, 2025 07:59

Fix method name

f685297

Test: Remove unused variable

788f3bc

smengcl force-pushed the HDDS-13006-yaml branch from 83a05e6 to 788f3bc Compare June 12, 2025 15:14

smengcl added 4 commits June 13, 2025 11:40

Make OmSnapshotLocalData abstract; clean up ser-de method in `OmSna…

efb4a4e

…pshotLocalDataYaml`

Fix UT usage

1dfdde0

Restore SstFilteringService

4864f15

Remove SnapshotLocalDataRepresenter since we are dumping ALL fields…

73c1702

…, not selective ones.

Clean up

c9589a1

jojochuang reviewed Jun 16, 2025

View reviewed changes

smengcl requested a review from swamirishi June 24, 2025 15:20

Add version field.

4c2d2ea

smengcl force-pushed the HDDS-13006-yaml branch from 54d7649 to 4c2d2ea Compare June 24, 2025 16:35

jojochuang requested review from SaketaChalamchala and jojochuang July 7, 2025 15:33

jojochuang approved these changes Jul 10, 2025

View reviewed changes

jojochuang marked this pull request as ready for review July 10, 2025 01:03

Fix Used undeclared dependencies.

cb6c469

Change-Id: I6f258b5f8eb08affd4438f640471d927a6599c3d

jojochuang merged commit e08b001 into apache:master Jul 10, 2025
42 checks passed

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jul 31, 2025

HDDS-13006. Use yaml files to host Ozone snapshot local properties (a…

49df6cf

…pache#8555) Co-authored-by: Wei-Chiu Chuang <[email protected]> Generated-by: Claude 3.7 Sonnet

HDDS-13006. Use yaml files to host Ozone snapshot local properties #8555

HDDS-13006. Use yaml files to host Ozone snapshot local properties #8555

Uh oh!

Conversation

smengcl commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

swamirishi commented Jun 3, 2025

Uh oh!

jojochuang commented Jun 4, 2025

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smengcl commented Jun 8, 2025

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swamirishi Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaketaChalamchala Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smengcl commented Jun 24, 2025

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jojochuang commented Jul 10, 2025

Uh oh!

smengcl commented Jun 3, 2025 •

edited

Loading

smengcl commented Jun 4, 2025 •

edited

Loading

swamirishi Jun 9, 2025 •

edited

Loading

swamirishi Jun 9, 2025 •

edited

Loading

SaketaChalamchala Jun 10, 2025 •

edited

Loading

swamirishi Jun 10, 2025 •

edited

Loading

smengcl commented Jun 9, 2025 •

edited

Loading

smengcl Jun 17, 2025 •

edited

Loading