HDDS-13798. Implement PoolBasedHierarchicalResourceLockManager for Hierarchical Resource by swamirishi · Pull Request #9160 · apache/ozone

swamirishi · 2025-10-15T16:42:54Z

What changes were proposed in this pull request?

Have PoolBasedHierarchicalResourceLockManager which would implement the class
HierachicalResourceLockManager backed by generic object pool and ConcurrentHashMap enabling acquiring read write locks on resources with a deterministic order which is not possible with a stripe based locking.
This is required for acquiring locks on chain while iterating through the snapshot chain.
FYI this can be reused for FSO path locking while path resolution.(OM leader execution charter)

What is the link to the Apache JIRA

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

How was this patch tested?

Added unit test

… hierarchical resource locking Change-Id: I433a52feb491b72ea303fa32025540a742555d08

Change-Id: I545cb5fa8ab7a2e2fb902b3559cbf2f48f84ad59

Change-Id: I5ffd4cff6028b50c8d75ea9b3885c1e9818fe968

…erarchical Resource Change-Id: Iabeb0c8a90500ed9f6a57e232470d20f7c7251bf

Change-Id: I9cfe0a545a5d4565b6a5e6fb94ea86f29d0f23ad

Change-Id: I3a88ac8556dc0238e67978944d7800bfb28ff41b

Change-Id: I4900bb92e6ccc3f728f4c61261c7accc7a8d4043

Change-Id: I6953c4f0fab1b0b54e1b4f1fce69025fdd424ac9

Change-Id: I31407994bb2d717f22977730b8b33ebc6eb33eea

Change-Id: I8b0d1657f809bf79cc5c5a64b336b7c6689aee91

Copilot

Pull Request Overview

This PR implements a new pool-based hierarchical resource lock manager for Ozone Manager that enables deterministic lock ordering for hierarchical resources, primarily for chain locking during snapshot operations and FSO path locking.

Key changes:

Introduces PoolBasedHierarchicalResourceLockManager using Apache Commons Pool2 for reusable ReadWriteLock instances
Adds ReadOnlyHierarchicalResourceLockManager for read-only scenarios
Integrates the hierarchical lock manager into the OM metadata manager infrastructure

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`PoolBasedHierarchicalResourceLockManager.java`	Core implementation using object pooling and ConcurrentHashMap for hierarchical resource locking
`ReadOnlyHierarchicalResourceLockManager.java`	Read-only implementation that returns no-op locks
`TestPoolBasedHierarchicalResourceLockManager.java`	Comprehensive test suite covering basic operations, concurrency, and stress scenarios
`OmMetadataManagerImpl.java`	Integration of hierarchical lock manager into OM metadata management
`OMMetadataManager.java`	Interface addition for hierarchical lock manager access
`OMConfigKeys.java`	Configuration constants for pool size limits
`ozone-default.xml`	Default configuration values for lock pool parameters
`pom.xml`	Added commons-pool2 dependency

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-16T06:24:02Z

...n/src/main/java/org/apache/hadoop/ozone/om/lock/ReadOnlyHierarchicalResourceLockManager.java

+  private static final HierarchicalResourceLock EMPTY_LOCK_NOT_ACQUIRED = new HierarchicalResourceLock() {
+    @Override
+    public boolean isLockAcquired() {
+      return true;


The EMPTY_LOCK_NOT_ACQUIRED constant incorrectly returns true for isLockAcquired(). This should return false to indicate the lock was not acquired, which is consistent with its name and usage context for write locks in read-only scenarios.

Suggested change

return true;

return false;

Copilot · 2025-10-16T06:24:02Z

hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java

 import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
 import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo;
 import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.lock.HierachicalResourceLockManager;


Corrected spelling of 'Hierachical' to 'Hierarchical'."

Copilot · 2025-10-16T06:24:02Z

hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java

+  /**
+   * Returns the Hierarchical ResourceLock used on Metadata DB.
+   */
+  HierachicalResourceLockManager getHierarchicalLockManager();


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Copilot · 2025-10-16T06:24:03Z

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

 import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
 import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
 import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+import org.apache.hadoop.ozone.om.lock.HierachicalResourceLockManager;


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Copilot · 2025-10-16T06:24:03Z

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

  private DBStore store;

  private final IOzoneManagerLock lock;
+  private final HierachicalResourceLockManager hierarchicalLockManager;


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Copilot · 2025-10-16T06:24:03Z

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

  }

+  @Override
+  public HierachicalResourceLockManager getHierarchicalLockManager() {


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Copilot · 2025-10-16T06:24:03Z

.../src/main/java/org/apache/hadoop/ozone/om/lock/PoolBasedHierarchicalResourceLockManager.java

+ * lock dependencies, and is typically useful for structures like
+ * DAGs (e.g., File System trees or snapshot chains).
+ */
+public class PoolBasedHierarchicalResourceLockManager implements HierachicalResourceLockManager {


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Suggested change

public class PoolBasedHierarchicalResourceLockManager implements HierachicalResourceLockManager {

public class PoolBasedHierarchicalResourceLockManager implements HierarchicalResourceLockManager {

Copilot · 2025-10-16T06:24:04Z

...n/src/main/java/org/apache/hadoop/ozone/om/lock/ReadOnlyHierarchicalResourceLockManager.java

+/**
+ * A read only lock manager that does not acquire any lock.
+ */
+public class ReadOnlyHierarchicalResourceLockManager implements HierachicalResourceLockManager {


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Suggested change

public class ReadOnlyHierarchicalResourceLockManager implements HierachicalResourceLockManager {

public class ReadOnlyHierarchicalResourceLockManager implements HierarchicalResourceLockManager {

Copilot · 2025-10-16T06:24:04Z

.../test/java/org/apache/hadoop/ozone/om/lock/TestPoolBasedHierarchicalResourceLockManager.java

+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.om.lock.HierachicalResourceLockManager.HierarchicalResourceLock;


Corrected spelling of 'HierachicalResourceLockManager' to 'HierarchicalResourceLockManager'."

Suggested change

import org.apache.hadoop.ozone.om.lock.HierachicalResourceLockManager.HierarchicalResourceLock;

import org.apache.hadoop.ozone.om.lock.HierarchicalResourceLockManager.HierarchicalResourceLock;

jojochuang · 2025-10-16T06:37:27Z

.../test/java/org/apache/hadoop/ozone/om/lock/TestPoolBasedHierarchicalResourceLockManager.java

+  /**
+   * Test different resource types can be locked independently.
+   */
+  @ParameterizedTest
+  @EnumSource(FlatResource.class)
+  public void testDifferentResourceTypes(FlatResource resource) throws Exception {
+    String key = "test-key-" + resource.name();
+
+    try (HierarchicalResourceLock lock = lockManager.acquireWriteLock(resource, key)) {
+      assertNotNull(lock);
+      assertTrue(lock.isLockAcquired());
+    }
+  }


this test doesn't seem to do what it supposed to do.
Instead, it should verify:

try (HierarchicalResourceLock lock = lockManager.acquireWriteLock(SNAPSHOT_GC_LOCK, key)) { assertNotNull(lock); assertTrue(lock.isLockAcquired()); try (HierarchicalResourceLock lock2 = lockManager.acquireWriteLock(SNAPSHOT_DB_LOCK, key)) { assertNotNull(lock2); assertTrue(lock2.isLockAcquired()); } }

so that it ensures the two locks can be held independently at the same time.

jojochuang · 2025-10-16T06:59:37Z

.../test/java/org/apache/hadoop/ozone/om/lock/TestPoolBasedHierarchicalResourceLockManager.java

+  }
+
+  /**
+   * Test configuration parameters are respected.


the test implementation itself does not ensure soft limit and hard limit are respected, only that a HierarchicalResourceLock is acquired without exceptions.

Done updated the tests

jojochuang · 2025-10-16T07:05:17Z

...n/src/main/java/org/apache/hadoop/ozone/om/lock/ReadOnlyHierarchicalResourceLockManager.java

+  private static final HierarchicalResourceLock EMPTY_LOCK_NOT_ACQUIRED = new HierarchicalResourceLock() {
+    @Override
+    public boolean isLockAcquired() {
+      return true;


Nice catch. yeah this was a typo. Fixed it

jojochuang · 2025-10-16T07:05:55Z

...n/src/main/java/org/apache/hadoop/ozone/om/lock/ReadOnlyHierarchicalResourceLockManager.java

+import java.io.IOException;
+
+/**
+ * A read only lock manager that does not acquire any lock.


i'm curious how this is going to be used because it looks basically like a no-op

Yeah this would be used when opening checkpoint metadata managers similar to OmReadOnlyLocks. Eventually we might want to use the HierarchicalLock for FSO kinda use case so created this along the same lines to OzoneManagerLock

Change-Id: I39115b2cb142fd36e370c8c5e72d6e58ce1ffb3a

Change-Id: Ie4b4b50c28aa5c62eeae51feec99304fec4b079e

Change-Id: I4625b940c0fd242897d2e1a3ae299c001588be1b

jojochuang

LGTM merge to unblock the rest.

swamirishi · 2025-10-16T15:35:08Z

@kerneltime @sumitagrawl @errose28 This can be used for FSO locking usecase for path resolution. We are planning to use this thing similarly #9140 for acquiring read locks on specific snapshot while iterating through the chain.

swamirishi added 4 commits October 15, 2025 09:45

HDDS-13797. Refactor OzoneManagerLock Resource class to handle handle…

915562b

… hierarchical resource locking Change-Id: I433a52feb491b72ea303fa32025540a742555d08

Merge remote-tracking branch 'apache/master' into HEAD

1c0d0ac

Change-Id: I545cb5fa8ab7a2e2fb902b3559cbf2f48f84ad59

HDDS-13797. Update interface

24da3eb

Change-Id: I5ffd4cff6028b50c8d75ea9b3885c1e9818fe968

HDDS-13798. Implement PoolBasedHierarchicalResourceLockManager for Hi…

8f3774a

…erarchical Resource Change-Id: Iabeb0c8a90500ed9f6a57e232470d20f7c7251bf

swamirishi requested review from jojochuang and smengcl October 15, 2025 16:43

swamirishi mentioned this pull request Oct 15, 2025

HDDS-13783. Implement locks for OmSnapshotLocalDataManager #9140

Merged

swamirishi added 2 commits October 15, 2025 15:38

HDDS-13797. Revert move of Leveled Resource and Resource enum/interface

6865fad

Change-Id: I9cfe0a545a5d4565b6a5e6fb94ea86f29d0f23ad

Merge remote-tracking branch 'origin/HDDS-13797' into HEAD

903ecd1

Change-Id: I3a88ac8556dc0238e67978944d7800bfb28ff41b

swamirishi added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Oct 15, 2025

swamirishi added 4 commits October 15, 2025 18:05

Merge remote-tracking branch 'apache/master' into HEAD

60a7728

Change-Id: I4900bb92e6ccc3f728f4c61261c7accc7a8d4043

HDDS-13798. Fix pmd findbugs

4711517

Change-Id: I6953c4f0fab1b0b54e1b4f1fce69025fdd424ac9

HDDS-13798. Fix pmd findbugs

655a724

Change-Id: I31407994bb2d717f22977730b8b33ebc6eb33eea

HDDS-13798. Fix ozone-default.xml

2bc6134

Change-Id: I8b0d1657f809bf79cc5c5a64b336b7c6689aee91

swamirishi marked this pull request as ready for review October 16, 2025 01:16

jojochuang requested a review from Copilot October 16, 2025 06:22

Copilot AI reviewed Oct 16, 2025

View reviewed changes

jojochuang reviewed Oct 16, 2025

View reviewed changes

swamirishi added 3 commits October 16, 2025 06:05

HDDS-13798. Stop lock data manager on metadata stop

8e8c534

Change-Id: I39115b2cb142fd36e370c8c5e72d6e58ce1ffb3a

HDDS-13798. Update tests

f148f24

Change-Id: Ie4b4b50c28aa5c62eeae51feec99304fec4b079e

HDDS-13798. Rename class

da030c0

Change-Id: I4625b940c0fd242897d2e1a3ae299c001588be1b

swamirishi requested a review from jojochuang October 16, 2025 14:38

jojochuang approved these changes Oct 16, 2025

View reviewed changes

jojochuang merged commit 1e7067a into apache:master Oct 16, 2025
43 checks passed

	public class PoolBasedHierarchicalResourceLockManager implements HierachicalResourceLockManager {
	public class PoolBasedHierarchicalResourceLockManager implements HierarchicalResourceLockManager {

	public class ReadOnlyHierarchicalResourceLockManager implements HierachicalResourceLockManager {
	public class ReadOnlyHierarchicalResourceLockManager implements HierarchicalResourceLockManager {

	import org.apache.hadoop.ozone.om.lock.HierachicalResourceLockManager.HierarchicalResourceLock;
	import org.apache.hadoop.ozone.om.lock.HierarchicalResourceLockManager.HierarchicalResourceLock;

Conversation

swamirishi commented Oct 15, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 16, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swamirishi commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants