This is an automated email from the ASF dual-hosted git repository.
szetszwo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new d6012190454 HDDS-15907. Do not use Collection in IOzoneManagerLock.
(#10907)
d6012190454 is described below
commit d601219045421721d52f0e00cf23bf7c67280fd6
Author: Tsz-Wo Nicholas Sze <[email protected]>
AuthorDate: Fri Jul 31 11:37:34 2026 -0700
HDDS-15907. Do not use Collection in IOzoneManagerLock. (#10907)
---
.../hadoop/ozone/om/lock/IOzoneManagerLock.java | 12 +-
.../hadoop/ozone/om/lock/OmReadOnlyLock.java | 10 +-
.../hadoop/ozone/om/lock/OzoneManagerLock.java | 163 ++++++++++++++-------
.../snapshot/OMSnapshotMoveTableKeysResponse.java | 7 +-
.../TestOMSnapshotMoveTableKeysResponse.java | 6 +-
5 files changed, 123 insertions(+), 75 deletions(-)
diff --git
a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java
b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java
index 66029caf7ff..e9319a787e7 100644
---
a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java
+++
b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java
@@ -18,7 +18,6 @@
package org.apache.hadoop.ozone.om.lock;
import com.google.common.annotations.VisibleForTesting;
-import java.util.Collection;
import java.util.concurrent.atomic.AtomicBoolean;
import org.apache.ratis.util.function.UncheckedAutoCloseableSupplier;
@@ -30,13 +29,12 @@ public interface IOzoneManagerLock {
OMLockDetails acquireReadLock(Resource resource,
String... resources);
- OMLockDetails acquireReadLocks(Resource resource, Collection<String[]>
resources);
+ OMLockDetails acquireReadLocks(Resource resource, Iterable<String[]> keys);
OMLockDetails acquireWriteLock(Resource resource,
String... resources);
- OMLockDetails acquireWriteLocks(Resource resource,
- Collection<String[]> resources);
+ OMLockDetails acquireWriteLocks(Resource resource, Iterable<String[]> keys);
OMLockDetails acquireResourceWriteLock(Resource resource);
@@ -47,16 +45,14 @@ OMLockDetails acquireWriteLocks(Resource resource,
OMLockDetails releaseWriteLock(Resource resource,
String... resources);
- OMLockDetails releaseWriteLocks(Resource resource,
- Collection<String[]> resources);
+ OMLockDetails releaseWriteLocks(Resource resource, Iterable<String[]> keys);
OMLockDetails releaseResourceWriteLock(Resource resource);
OMLockDetails releaseReadLock(Resource resource,
String... resources);
- OMLockDetails releaseReadLocks(Resource resource,
- Collection<String[]> resources);
+ OMLockDetails releaseReadLocks(Resource resource, Iterable<String[]> keys);
@VisibleForTesting
int getReadHoldCount(Resource resource,
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java
index faf5ca99b8c..7e544b01020 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmReadOnlyLock.java
@@ -20,8 +20,6 @@
import static
org.apache.hadoop.ozone.om.lock.OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED;
import static
org.apache.hadoop.ozone.om.lock.OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
-import java.util.Collection;
-
/**
* Read only "lock" for snapshots
* Uses no lock. Always returns true when acquiring
@@ -35,7 +33,7 @@ public OMLockDetails acquireReadLock(Resource resource,
String... resources) {
}
@Override
- public OMLockDetails acquireReadLocks(Resource resource,
Collection<String[]> resources) {
+ public OMLockDetails acquireReadLocks(Resource resource, Iterable<String[]>
keys) {
return EMPTY_DETAILS_LOCK_ACQUIRED;
}
@@ -46,7 +44,7 @@ public OMLockDetails acquireWriteLock(Resource resource,
}
@Override
- public OMLockDetails acquireWriteLocks(Resource resource,
Collection<String[]> resources) {
+ public OMLockDetails acquireWriteLocks(Resource resource, Iterable<String[]>
keys) {
return EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
}
@@ -72,7 +70,7 @@ public OMLockDetails releaseWriteLock(Resource resource,
}
@Override
- public OMLockDetails releaseWriteLocks(Resource resource,
Collection<String[]> resources) {
+ public OMLockDetails releaseWriteLocks(Resource resource, Iterable<String[]>
keys) {
return EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
}
@@ -87,7 +85,7 @@ public OMLockDetails releaseReadLock(Resource resource,
String... resources) {
}
@Override
- public OMLockDetails releaseReadLocks(Resource resource,
Collection<String[]> resources) {
+ public OMLockDetails releaseReadLocks(Resource resource, Iterable<String[]>
keys) {
return EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
}
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
index b0abd85f944..a6a47844519 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
@@ -24,26 +24,27 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.util.concurrent.Striped;
-import java.util.ArrayList;
import java.util.Arrays;
-import java.util.Collection;
import java.util.Collections;
+import java.util.Deque;
import java.util.EnumMap;
+import java.util.Iterator;
+import java.util.LinkedList;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
+import java.util.RandomAccess;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantReadWriteLock;
-import java.util.function.Function;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
import java.util.stream.Collectors;
-import java.util.stream.IntStream;
-import java.util.stream.StreamSupport;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.utils.CompositeKey;
import org.apache.hadoop.hdds.utils.SimpleStriped;
import org.apache.hadoop.ipc_.ProcessingDetails.Timing;
import org.apache.hadoop.ipc_.Server;
import org.apache.hadoop.util.Time;
+import org.apache.ratis.util.CollectionUtils;
import org.apache.ratis.util.Preconditions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -140,24 +141,38 @@ private void acquireLock(R resource, boolean isRead,
ReentrantReadWriteLock lock
}
}
- OMLockDetails acquire(Resource resource, boolean isRead,
- Function<Striped<ReentrantReadWriteLock>,
Iterable<ReentrantReadWriteLock>> getLocks) {
+ private OMLockDetails acquireImpl(Resource resource, BiConsumer<R, Long>
acquireLockMethod) {
final R r = assertAcquire(resource);
final long startWaitingTimeNanos = Time.monotonicNowNanos();
- for (ReentrantReadWriteLock lock : getLocks.apply(lockMap.get(r))) {
- acquireLock(r, isRead, lock, startWaitingTimeNanos);
- }
+ acquireLockMethod.accept(r, startWaitingTimeNanos);
return tracker.lockResource(r);
}
- OMLockDetails acquire(Resource resource, boolean isRead, String... keys) {
- final R r = assertAcquire(resource);
- final long startWaitingTimeNanos = Time.monotonicNowNanos();
- acquireLock(r, isRead, getLock(r, keys), startWaitingTimeNanos);
- return tracker.lockResource(r);
+ private OMLockDetails acquireOne(Resource resource, boolean isRead,
String... keys) {
+ return acquireImpl(resource, (r, startWaitingTimeNanos) -> {
+ final ReentrantReadWriteLock lock = getLock(r, keys);
+ acquireLock(r, isRead, lock, startWaitingTimeNanos);
+ });
}
- void releaseLock(R resource, boolean isRead, ReentrantReadWriteLock lock) {
+ private OMLockDetails acquireAll(Resource resource) {
+ return acquireImpl(resource, (r, startWaitingTimeNanos) -> {
+ final Striped<ReentrantReadWriteLock> striped = lockMap.get(r);
+ for (int i = 0; i < striped.size(); i++) {
+ acquireLock(r, false, striped.getAt(i), startWaitingTimeNanos);
+ }
+ });
+ }
+
+ private OMLockDetails acquireSelected(Resource resource, boolean isRead,
Iterable<String[]> keys) {
+ return acquireImpl(resource, (r, startWaitingTimeNanos) -> {
+ for (ReentrantReadWriteLock lock : bulkGetForAcquire(lockMap.get(r),
keys)) {
+ acquireLock(r, isRead, lock, startWaitingTimeNanos);
+ }
+ });
+ }
+
+ private void releaseLock(R resource, boolean isRead,
ReentrantReadWriteLock lock) {
if (isRead) {
lock.readLock().unlock();
updateReadUnlockMetrics(resource, tracker, lock);
@@ -168,27 +183,36 @@ void releaseLock(R resource, boolean isRead,
ReentrantReadWriteLock lock) {
}
}
- OMLockDetails release(Resource resource, boolean isRead, String... keys) {
+ private OMLockDetails releaseImpl(Resource resource, Consumer<R>
releaseLockMethod) {
final R r = Preconditions.assertInstanceOf(resource,
tracker.getResourceClass());
tracker.clearLockDetails();
- final ReentrantReadWriteLock lock = getLock(r, keys);
- releaseLock(r, isRead, lock);
+ releaseLockMethod.accept(r);
return tracker.unlockResource(r);
}
- private OMLockDetails release(Resource resource, boolean isRead,
- Function<Striped<ReentrantReadWriteLock>,
Iterable<ReentrantReadWriteLock>> getLock) {
- final R r = Preconditions.assertInstanceOf(resource,
tracker.getResourceClass());
- tracker.clearLockDetails();
- final Iterable<ReentrantReadWriteLock> i = getLock.apply(lockMap.get(r));
- final List<ReentrantReadWriteLock> locks =
StreamSupport.stream(i.spliterator(), false)
- .collect(Collectors.toList());
- // Release locks in reverse order.
- Collections.reverse(locks);
- for (ReentrantReadWriteLock lock : locks) {
+ private OMLockDetails releaseOne(Resource resource, boolean isRead,
String... keys) {
+ return releaseImpl(resource, r -> {
+ final ReentrantReadWriteLock lock = getLock(r, keys);
releaseLock(r, isRead, lock);
- }
- return tracker.unlockResource(r);
+ });
+ }
+
+ private OMLockDetails releaseAll(Resource resource) {
+ return releaseImpl(resource, r -> {
+ final Striped<ReentrantReadWriteLock> striped = lockMap.get(r);
+ // Release locks in reverse order.
+ for (int i = striped.size() - 1; i >= 0; i--) {
+ releaseLock(r, false, striped.getAt(i));
+ }
+ });
+ }
+
+ private OMLockDetails releaseSelected(Resource resource, boolean isRead,
Iterable<String[]> keys) {
+ return releaseImpl(resource, r -> {
+ for (ReentrantReadWriteLock lock : bulkGetForRelease(lockMap.get(r),
keys)) {
+ releaseLock(r, isRead, lock);
+ }
+ });
}
List<String> getCurrentLocks() {
@@ -237,19 +261,48 @@ private static Striped<ReentrantReadWriteLock>
createStripeLock(Resource r, Conf
return SimpleStriped.readWriteLock(size, fair);
}
- private Iterable<ReentrantReadWriteLock>
getAllLocks(Striped<ReentrantReadWriteLock> striped) {
- return IntStream.range(0,
striped.size()).mapToObj(striped::getAt).collect(Collectors.toList());
+ /** @return locks in ascending order for acquire. */
+ static Iterable<ReentrantReadWriteLock> bulkGetForAcquire(
+ Striped<ReentrantReadWriteLock> striped, Iterable<String[]> keys) {
+ return striped.bulkGet(CollectionUtils.as(keys,
CompositeKey::combineKeys)); // no copying
}
- private Iterable<ReentrantReadWriteLock>
bulkGetLock(Striped<ReentrantReadWriteLock> striped,
- Collection<String[]> keys) {
- List<Object> lockKeys = new ArrayList<>(keys.size());
- for (String[] key : keys) {
- if (Objects.nonNull(key)) {
- lockKeys.add(CompositeKey.combineKeys(key));
+ /** @return locks in descending order for release. */
+ static Iterable<ReentrantReadWriteLock> bulkGetForRelease(
+ Striped<ReentrantReadWriteLock> striped, Iterable<String[]> keys) {
+ final Iterable<ReentrantReadWriteLock> iterable =
bulkGetForAcquire(striped, keys);
+
+ // although the return type of Striped.bulkGet(..) is Iterable, its
implementation currently returns an ArrayList.
+ if (iterable instanceof List && iterable instanceof RandomAccess) {
+ final List<ReentrantReadWriteLock> list = (List<ReentrantReadWriteLock>)
iterable;
+ // return in descending order
+ return () -> new Iterator<ReentrantReadWriteLock>() {
+ private int i = list.size() - 1;
+
+ @Override
+ public boolean hasNext() {
+ return i >= 0;
+ }
+
+ @Override
+ public ReentrantReadWriteLock next() {
+ return list.get(i--);
+ }
+ };
+ }
+
+ // use Deque
+ final Deque<ReentrantReadWriteLock> deque;
+ if (iterable instanceof Deque) {
+ deque = (Deque<ReentrantReadWriteLock>) iterable;
+ } else {
+ // fallback copying to a list
+ deque = new LinkedList<>();
+ for (ReentrantReadWriteLock lock : iterable) {
+ deque.add(lock);
}
}
- return striped.bulkGet(lockKeys);
+ return deque::descendingIterator;
}
/**
@@ -272,7 +325,7 @@ private Iterable<ReentrantReadWriteLock>
bulkGetLock(Striped<ReentrantReadWriteL
@Override
public OMLockDetails acquireReadLock(Resource resource, String... keys) {
return getResourceLocks(resource)
- .acquire(resource, true, keys);
+ .acquireOne(resource, true, keys);
}
/**
@@ -293,9 +346,9 @@ public OMLockDetails acquireReadLock(Resource resource,
String... keys) {
* be passed.
*/
@Override
- public OMLockDetails acquireReadLocks(Resource resource,
Collection<String[]> keys) {
+ public OMLockDetails acquireReadLocks(Resource resource, Iterable<String[]>
keys) {
return getResourceLocks(resource)
- .acquire(resource, true, striped -> bulkGetLock(striped, keys));
+ .acquireSelected(resource, true, keys);
}
/**
@@ -318,7 +371,7 @@ public OMLockDetails acquireReadLocks(Resource resource,
Collection<String[]> ke
@Override
public OMLockDetails acquireWriteLock(Resource resource, String... keys) {
return getResourceLocks(resource)
- .acquire(resource, false, keys);
+ .acquireOne(resource, false, keys);
}
/**
@@ -339,9 +392,9 @@ public OMLockDetails acquireWriteLock(Resource resource,
String... keys) {
* be passed.
*/
@Override
- public OMLockDetails acquireWriteLocks(Resource resource,
Collection<String[]> keys) {
+ public OMLockDetails acquireWriteLocks(Resource resource, Iterable<String[]>
keys) {
return getResourceLocks(resource)
- .acquire(resource, false, striped -> bulkGetLock(striped, keys));
+ .acquireSelected(resource, false, keys);
}
/**
@@ -352,7 +405,7 @@ public OMLockDetails acquireWriteLocks(Resource resource,
Collection<String[]> k
@Override
public OMLockDetails acquireResourceWriteLock(Resource resource) {
return getResourceLocks(resource)
- .acquire(resource, false, this::getAllLocks);
+ .acquireAll(resource);
}
private void updateReadLockMetrics(Resource resource, ResourceLockTracker<?
extends Resource> tracker,
@@ -433,7 +486,7 @@ public void releaseMultiUserLock(String firstUser, String
secondUser) {
@Override
public OMLockDetails releaseWriteLock(Resource resource, String... keys) {
return getResourceLocks(resource)
- .release(resource, false, keys);
+ .releaseOne(resource, false, keys);
}
/**
@@ -445,9 +498,9 @@ public OMLockDetails releaseWriteLock(Resource resource,
String... keys) {
* be passed.
*/
@Override
- public OMLockDetails releaseWriteLocks(Resource resource,
Collection<String[]> keys) {
+ public OMLockDetails releaseWriteLocks(Resource resource, Iterable<String[]>
keys) {
return getResourceLocks(resource)
- .release(resource, false, striped -> bulkGetLock(striped, keys));
+ .releaseSelected(resource, false, keys);
}
/**
@@ -458,7 +511,7 @@ public OMLockDetails releaseWriteLocks(Resource resource,
Collection<String[]> k
@Override
public OMLockDetails releaseResourceWriteLock(Resource resource) {
return getResourceLocks(resource)
- .release(resource, false, this::getAllLocks);
+ .releaseAll(resource);
}
/**
@@ -472,7 +525,7 @@ public OMLockDetails releaseResourceWriteLock(Resource
resource) {
@Override
public OMLockDetails releaseReadLock(Resource resource, String... keys) {
return getResourceLocks(resource)
- .release(resource, true, keys);
+ .releaseOne(resource, true, keys);
}
/**
@@ -484,9 +537,9 @@ public OMLockDetails releaseReadLock(Resource resource,
String... keys) {
* be passed.
*/
@Override
- public OMLockDetails releaseReadLocks(Resource resource,
Collection<String[]> keys) {
+ public OMLockDetails releaseReadLocks(Resource resource, Iterable<String[]>
keys) {
return getResourceLocks(resource)
- .release(resource, true, striped -> bulkGetLock(striped, keys));
+ .releaseSelected(resource, true, keys);
}
private void updateReadUnlockMetrics(Resource resource,
ResourceLockTracker<? extends Resource> tracker,
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveTableKeysResponse.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveTableKeysResponse.java
index c9ed469d6ca..e70b35d9403 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveTableKeysResponse.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveTableKeysResponse.java
@@ -21,9 +21,10 @@
import static
org.apache.hadoop.ozone.om.lock.DAGLeveledResource.SNAPSHOT_DB_CONTENT_LOCK;
import static
org.apache.hadoop.ozone.om.snapshot.SnapshotUtils.createMergedRepeatedOmKeyInfoFromDeletedTableEntry;
-import com.google.common.collect.Lists;
import jakarta.annotation.Nonnull;
import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.utils.db.BatchOperation;
@@ -87,8 +88,8 @@ protected void addToDBBatch(OMMetadataManager
omMetadataManager, BatchOperation
.getOzoneManager().getOmSnapshotManager();
IOzoneManagerLock lock = omMetadataManager.getLock();
String[] fromSnapshotId = new String[]
{fromSnapshot.getSnapshotId().toString()};
- String[] nextSnapshotId = nextSnapshot == null ? null : new String[]
{nextSnapshot.getSnapshotId().toString()};
- List<String[]> snapshotIds = Lists.newArrayList(fromSnapshotId,
nextSnapshotId);
+ final List<String[]> snapshotIds = nextSnapshot == null ?
Collections.singletonList(fromSnapshotId)
+ : Arrays.asList(fromSnapshotId, new
String[]{nextSnapshot.getSnapshotId().toString()});
OMLockDetails lockDetails =
lock.acquireReadLocks(SNAPSHOT_DB_CONTENT_LOCK, snapshotIds);
if (!lockDetails.isLockAcquired()) {
throw new OMException("Unable to acquire read lock on " +
SNAPSHOT_DB_CONTENT_LOCK + " for snapshot: " +
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotMoveTableKeysResponse.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotMoveTableKeysResponse.java
index eb3d2a6612a..696f9ac4f6b 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotMoveTableKeysResponse.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotMoveTableKeysResponse.java
@@ -128,9 +128,9 @@ public void testMoveTableKeysToNextSnapshot(boolean
nextSnapshotExists) throws E
getVolumeName(), getBucketName(), snapshotName1);
UncheckedAutoCloseableSupplier<OmSnapshot> snapshot2 =
nextSnapshotExists ? getOmSnapshotManager().getSnapshot(
getVolumeName(), getBucketName(), snapshotName2) : null) {
- List<List<String>> expectedSnapshotIdLocks =
-
Arrays.asList(Collections.singletonList(snapshot1.get().getSnapshotID().toString()),
- nextSnapshotExists ?
Collections.singletonList(snapshot2.get().getSnapshotID().toString()) : null);
+ final List<String> first =
Collections.singletonList(snapshot1.get().getSnapshotID().toString());
+ final List<List<String>> expectedSnapshotIdLocks = !nextSnapshotExists ?
Collections.singletonList(first)
+ : Arrays.asList(first,
Collections.singletonList(snapshot2.get().getSnapshotID().toString()));
List<List<String>> locks = new ArrayList<>();
doAnswer(i -> {
for (String[] id : (Collection<String[]>)i.getArgument(1)) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
