hemantk-12 commented on code in PR #8052:
URL: https://github.com/apache/ozone/pull/8052#discussion_r2017991215
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java:
##########
@@ -122,6 +126,12 @@ private Striped<ReadWriteLock> createStripeLock(Resource r,
return SimpleStriped.readWriteLock(size, fair);
}
+ private Iterable<ReadWriteLock> bulkGetLock(Resource resource,
Collection<String[]> keys) {
+ Striped<ReadWriteLock> striped = stripedLockByResource.get(resource);
+ return striped.bulkGet(keys.stream().filter(Objects::nonNull)
Review Comment:
nit: Java Streams are better suited for scenarios where multiple operations
are performed, enabling lazy loading. For a simple scenario like this, a for
loop is cheaper than a stream.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java:
##########
@@ -172,6 +204,54 @@ public OMLockDetails acquireWriteLock(Resource resource,
String... keys) {
return acquireLock(resource, false, keys);
}
+ /**
+ * Acquire write locks on a list of resources.
+ *
+ * For S3_BUCKET_LOCK, VOLUME_LOCK, BUCKET_LOCK type resource, same
+ * thread acquiring lock again is allowed.
+ *
+ * For USER_LOCK, PREFIX_LOCK, S3_SECRET_LOCK type resource, same thread
+ * acquiring lock again is not allowed.
+ *
+ * Special Note for USER_LOCK: Single thread can acquire single user lock/
+ * multi user lock. But not both at the same time.
+ * @param resource - Type of the resource.
+ * @param keys - A list of Resource names on which user want to acquire lock.
+ * For Resource type BUCKET_LOCK, first param should be volume, second param
+ * should be bucket name. For remaining all resource only one param should
+ * be passed.
+ */
+ @Override
+ public OMLockDetails acquireWriteLocks(Resource resource,
Collection<String[]> keys) {
+ return acquireLocks(resource, false, keys);
+ }
+
+ private OMLockDetails acquireLocks(Resource resource, boolean isReadLock,
+ Collection<String[]> keys) {
+ omLockDetails.get().clear();
+ if (!resource.canLock(lockSet.get())) {
+ String errorMessage = getErrorMessage(resource);
+ LOG.error(errorMessage);
+ throw new RuntimeException(errorMessage);
+ }
+
+ long startWaitingTimeNanos = Time.monotonicNowNanos();
+
+ for (ReadWriteLock lock : bulkGetLock(resource, keys)) {
+ if (isReadLock) {
+ lock.readLock().lock();
+ updateReadLockMetrics(resource, (ReentrantReadWriteLock) lock,
startWaitingTimeNanos);
+ } else {
+ lock.writeLock().lock();
+ updateWriteLockMetrics(resource, (ReentrantReadWriteLock) lock,
startWaitingTimeNanos);
+ }
Review Comment:
nit: create a helper function and use it here and
[here](https://github.com/apache/ozone/blob/43ab7b744e73e4b9bd91e91b3c8f6acaa6d16b48/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java#L266).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
+import org.apache.hadoop.ozone.om.lock.OMLockDetails;
+import org.apache.hadoop.ozone.om.lock.OzoneManagerLock;
+
+/**
+ * Class to take multiple locks on multiple snapshots.
+ */
+public class MultiSnapshotLocks {
+ private final List<String[]> objectLocks;
+ private final IOzoneManagerLock lock;
+ private final OzoneManagerLock.Resource resource;
+ private final boolean writeLock;
+ private OMLockDetails lockDetails;
+
+ public MultiSnapshotLocks(IOzoneManagerLock lock, OzoneManagerLock.Resource
resource, boolean writeLock) {
+ this.writeLock = writeLock;
+ this.resource = resource;
+ this.lock = lock;
+ this.objectLocks = new ArrayList<>();
+ this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
+ }
+
+ public OMLockDetails acquireLock(Collection<UUID> ids) throws OMException {
+ if (!objectLocks.isEmpty()) {
+ throw new OMException("More locks cannot be acquired when locks have
been already acquired. Locks acquired : "
+ +
objectLocks.stream().map(Arrays::toString).collect(Collectors.toList()),
+ OMException.ResultCodes.INTERNAL_ERROR);
+ }
+ List<String[]> keys =
+ ids.stream().filter(Objects::nonNull).map(id -> new String[]
{id.toString()})
+ .collect(Collectors.toList());
+ OMLockDetails omLockDetails = this.writeLock ?
lock.acquireWriteLocks(resource, keys) :
+ lock.acquireReadLocks(resource, keys);
+ if (omLockDetails.isLockAcquired()) {
+ objectLocks.addAll(keys);
+ }
+ this.lockDetails = omLockDetails;
+ return omLockDetails;
+ }
+
+ public void releaseLock() {
+ if (this.writeLock) {
+ lockDetails = lock.releaseWriteLocks(resource, this.objectLocks);
+ } else {
+ lockDetails = lock.releaseReadLocks(resource, this.objectLocks);
+ }
+ this.objectLocks.clear();
+ }
+
+ @VisibleForTesting
Review Comment:
nit: @adoroszlai raised good points not to use `@VisibleForTesting`
annotation, and is planning to remove it completely.
https://github.com/apache/ozone/pull/7974#discussion_r1981887223
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
+import org.apache.hadoop.ozone.om.lock.OMLockDetails;
+import org.apache.hadoop.ozone.om.lock.OzoneManagerLock;
+
+/**
+ * Class to take multiple locks on multiple snapshots.
+ */
+public class MultiSnapshotLocks {
+ private final List<String[]> objectLocks;
+ private final IOzoneManagerLock lock;
+ private final OzoneManagerLock.Resource resource;
+ private final boolean writeLock;
+ private OMLockDetails lockDetails;
+
+ public MultiSnapshotLocks(IOzoneManagerLock lock, OzoneManagerLock.Resource
resource, boolean writeLock) {
+ this.writeLock = writeLock;
+ this.resource = resource;
+ this.lock = lock;
+ this.objectLocks = new ArrayList<>();
+ this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
+ }
+
+ public OMLockDetails acquireLock(Collection<UUID> ids) throws OMException {
+ if (!objectLocks.isEmpty()) {
Review Comment:
Since you are doing this, does `acquireLock` need to be thread-safe?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
+import org.apache.hadoop.ozone.om.lock.OMLockDetails;
+import org.apache.hadoop.ozone.om.lock.OzoneManagerLock;
+
+/**
+ * Class to take multiple locks on multiple snapshots.
+ */
+public class MultiSnapshotLocks {
+ private final List<String[]> objectLocks;
+ private final IOzoneManagerLock lock;
+ private final OzoneManagerLock.Resource resource;
+ private final boolean writeLock;
+ private OMLockDetails lockDetails;
+
+ public MultiSnapshotLocks(IOzoneManagerLock lock, OzoneManagerLock.Resource
resource, boolean writeLock) {
+ this.writeLock = writeLock;
+ this.resource = resource;
+ this.lock = lock;
+ this.objectLocks = new ArrayList<>();
+ this.lockDetails = OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED;
+ }
+
+ public OMLockDetails acquireLock(Collection<UUID> ids) throws OMException {
+ if (!objectLocks.isEmpty()) {
+ throw new OMException("More locks cannot be acquired when locks have
been already acquired. Locks acquired : "
+ +
objectLocks.stream().map(Arrays::toString).collect(Collectors.toList()),
+ OMException.ResultCodes.INTERNAL_ERROR);
Review Comment:
nit: directly collect it to the string rather than a list and then a string.
```suggestion
throw new OMException("More locks cannot be acquired when locks have
already been acquired. Locks acquired: "
+ objectLocks.stream()
.map(Arrays::toString)
.collect(Collectors.joining(", ", "[", "]")),
OMException.ResultCodes.INTERNAL_ERROR);
```
I'm not sure how useful resource list is. I would simply check if
`lockDetails.isLockAcquired()`.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java:
##########
@@ -317,6 +397,11 @@ public OMLockDetails releaseWriteLock(Resource resource,
String... keys) {
return releaseLock(resource, false, keys);
}
+ @Override
+ public OMLockDetails releaseWriteLocks(Resource resource,
Collection<String[]> keys) {
Review Comment:
nit: missing java doc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
