HDDS-15909. Refactor OzoneManagerLock. by szetszwo · Pull Request #10852 · apache/ozone

szetszwo · 2026-07-23T16:32:32Z

What changes were proposed in this pull request?

//OzoneManagerLock
  private final Map<Class<? extends Resource>,
      Pair<Map<Resource, Striped<ReadWriteLock>>, ResourceLockTracker>> resourcelockMap;

The resourcelockMap above is

inefficient: use a Map (the outer map) for only two elements.
hard to read/understand: too many layers
Apache Commons Pair (see HDDS-15908).
Not type safe: ResourceLockTracker is generic

What is the link to the Apache JIRA

HDDS-15909

How was this patch tested?

By existing tests.

spacemonkd · 2026-07-23T19:54:25Z

-  private OMLockMetrics omLockMetrics;
+  private final OMLockMetrics omLockMetrics = OMLockMetrics.create();
+
+  class ResourceLocks<R extends Resource> {


Just a question. Here we are having three levels of nested classes:
OzoneManagerLock { ResourceLocks { Context }}

As far as I understand ResourceLocks only access the outer class for the metrics.

Could extracting this to a separate private class be better? Or maybe making it a static class and passing the metrics object might make it easier to read?

Context is not very useful. Let's remove it.

spacemonkd · 2026-07-23T20:00:36Z

+    return locks;
+  }
+
+  private <T extends Resource> ResourceLocks<T> getResourceLocks(Resource instance) {


Nit: Is it better to declare this as -

Suggested change

private <T extends Resource> ResourceLocks<T> getResourceLocks(Resource instance) {

private ResourceLocks<?> getResourceLocks(Resource instance) {

Because if we say it is a generic T then "T" has no relationship with instance. The compiler is picking T from wherever the call is being made for this method.

So if we have a scenario like (this won't happen I understand and it is hypothetical):

ResourceLocks<LeveledResource> locks = getResourceLocks(DAGLeveledResource.BOOTSTRAP_LOCK);

then T = LeveledResource and no ClassCastException is thrown here.

We do check in cast():

resource.getClass() != tracker.getResourceClass()

which I guess gives us the safety, so this is just a nit from my end.

That's a good point! ResourceLocks<?> seems better

spacemonkd · 2026-07-23T20:01:14Z

-    Map<LeveledResource, Striped<ReadWriteLock>> stripedLockMap = new EnumMap<>(LeveledResource.class);
-    for (LeveledResource r : LeveledResource.values()) {
-      stripedLockMap.put(r, createStripeLock(r, conf));
+  private  <T extends Resource> ResourceLocks<T> cast(ResourceLocks<?> resourceLocks) {


Nit: double space

Suggested change

private <T extends Resource> ResourceLocks<T> cast(ResourceLocks<?> resourceLocks) {

private <T extends Resource> ResourceLocks<T> cast(ResourceLocks<?> resourceLocks) {

The method is removed now.

spacemonkd · 2026-07-23T20:03:15Z

-        .flatMap(rlm -> ((ResourceLockTracker<? extends Resource>)rlm).getCurrentLockedResources())
-        .map(Resource::getName)
-        .collect(Collectors.toList());
+  int getCurrentLockSizeForTesting() {


This simplifies the method, but then in tests we will only see the count.
For example it might be something like: expected 1 but got 2.

Previously I think it would have given the list of the locks as well to help debug any test issues. This is removing that ability. Do you think it might be good to return the locks that are held as well to debug later on?

Previously I think it would have given the list ...

No, the test currently never uses the List.

+++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/lock/TestKeyPathLock.java @@ -216,7 +216,7 @@ private void testDiffKeyPathWriteLockMultiThreadingUtil( // Waiting for all the threads to be instantiated/to reach // acquireWriteLock. countDown.countDown(); - assertEquals(1, lock.getCurrentLocks().size()); + assertEquals(1, lock.getCurrentLockSizeForTesting());

spacemonkd · 2026-07-23T20:10:08Z

+      }
+
+      long getWaitingTimeNanos() {
+        return Time.monotonicNowNanos() - startWaitingTimeNanos;


As far as I understand this is inheriting the previous behaviour where all lock time was summed up.

So:
lock 1: 10ms
lock 2: 20ms
lock 3: 10ms

This would return the total time instead of per lock waiting time. However with the context class I would expect per lock wait to be accounted for. Maybe we can add a comment saying that this is still sum of the total wait time?

Context is removed, so the code is reverted back as before.

spacemonkd

Thanks for the PR @szetszwo. Overall it looks good to me, just had some questions, could you take a look?

szetszwo · 2026-07-23T21:19:04Z

@spacemonkd , thanks for review this! Just pushed a change to address your comments.

spacemonkd · 2026-07-24T14:49:43Z

+    OMLockDetails acquire(Resource resource, boolean isRead,
+        Function<Striped<ReentrantReadWriteLock>, Iterable<ReentrantReadWriteLock>> getLocks) {
+      final R r = assertAcquire(resource);
+      final long startWaitingTimeNanos = Time.monotonicNow();


monotonicNow() is in miliseconds, but we need to have nanoseconds?

Suggested change

final long startWaitingTimeNanos = Time.monotonicNow();

final long startWaitingTimeNanos = Time.monotonicNowNanos();

spacemonkd

Thanks for the update @szetszwo , just one comment. Apart from that this looks good to me.

HDDS-15909. Refactor OzoneManagerLock.

946c906

szetszwo requested a review from spacemonkd July 23, 2026 16:34

spacemonkd reviewed Jul 23, 2026

View reviewed changes

Address review comments

b25befe

spacemonkd reviewed Jul 24, 2026

View reviewed changes

	private <T extends Resource> ResourceLocks<T> getResourceLocks(Resource instance) {
	private ResourceLocks<?> getResourceLocks(Resource instance) {

	final long startWaitingTimeNanos = Time.monotonicNow();
	final long startWaitingTimeNanos = Time.monotonicNowNanos();

Uh oh!

Conversation

szetszwo commented Jul 23, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacemonkd left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacemonkd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants