You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@ozone.apache.org by "sumitagrawl (via GitHub)" <gi...@apache.org> on 2025/04/02 13:11:30 UTC

[PR] HDDS-12356. granular locking framework [ozone]

sumitagrawl opened a new pull request, #8217:
URL: https://github.com/apache/ozone/pull/8217

   ## What changes were proposed in this pull request?
   
   Granular locking framework for OBS for existing flow. Its just framework binding code flow but pending to call the lock in respective flow.
   
   locking is added for external request at entry point. This provides execution of request at leader and existing flow simultaneouly without impacting for cache.
   
   - refer obs-locking.md for `locking added for obs request` (HDDS-11898. design doc leader side execution)
   - refer leader-execution.md for `Step-by-step integration` of existing request (interoperability)
   
   Next PR will include:
   - integration of locking framework to flow
   - locking for obs key operation, bucket operation, volume operation and MPU cases
   - https://issues.apache.org/jira/browse/HDDS-12386 configuration for lock bucket length and timeout
   
   Parent Jira:
   https://issues.apache.org/jira/browse/HDDS-11900
   
   Its Parent for Epic;
   https://issues.apache.org/jira/browse/HDDS-11897
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-12356
   
   ## How was this patch tested?
   
   - UT cases added for lock
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2028117149


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();

Review Comment:
   As response for above comments, its required as,
   - it keep track of lock stats to be responsed to Hadoop Call metrics for each request



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "kerneltime (via GitHub)" <gi...@apache.org>.

kerneltime commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2790338638

   @sumitagrawl is away for a few days so the updates will be incoming after that.
   I think a feature branch should be okay, but I want it to contain small changes that get merged in. If the change is a single PR, it does raise questions about why we are doing a feature branch. The migration of request processing for Ozone needs to be done incrementally, and there will be a period when request processing is in mixed mode. 


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2028117582


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks);
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      locks.clear();
+      Thread.currentThread().interrupt();
+      throw new OMException("waiting for locks is interrupted", OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      locks.clear();
+      throw new OMException("Unable to get locks, timeout occurred", OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  /*
+  Optional: If we want more diagnostic info on the type of lock that failed to be acquired (volume, bucket, or key),
+  We can make the parameter a list of objects that wrap the Lock with information about its type.
+  Note that logging the specific volume, bucket or keys this lock was trying to acquire is not helpful and
+  misleading because collisions within the stripe lock might mean we are blocked on a request for a completely
+  different part of the namespace.
+  Obtaining the thread ID that we were waiting on would be more useful, but there is no easy way to do that.
+   */
+  private void acquireLocks(List<Lock> locks) throws TimeoutException, InterruptedException {
+    List<Lock> acquiredLocks = new ArrayList<>(locks.size());
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);
+        } catch (Throwable e) {
+          // We acquired this lock but were unable to add it to our acquired locks list.
+          lock.unlock();
+          releaseLocks(acquiredLocks);
+          throw e;
+        }
+      } else {
+        releaseLocks(acquiredLocks);
+        throw new TimeoutException("Failed to acquire lock after the given timeout.");
+      }
+    }
+  }
+
+  public void unlock(OmLockObject lockObject) {
+    releaseLocks(lockObject.getLocks());
+    lockStatsEnd(lockObject.getLockStats(), lockObject.getLockStatType());
+    lockObject.getLocks().clear();
+  }
+
+  private void releaseLocks(List<Lock> locks) {
+    ListIterator<Lock> reverseIterator = locks.listIterator(locks.size());
+    while (reverseIterator.hasPrevious()) {
+      Lock lock = reverseIterator.previous();
+      lock.unlock();
+    }
+  }
+
+  private static void lockStatsBegin(OmLockStats lockStats, long endTime, long startTime) {
+    lockStats.add(endTime - startTime, OmLockStats.Type.WAIT);
+    lockStats.setLockStartTime(endTime);
+  }
+
+  private static void lockStatsEnd(OmLockStats lockStats, OmLockStats.Type type) {
+    if (lockStats.getLockStartTime() > 0) {
+      lockStats.add(Time.monotonicNowNanos() - lockStats.getLockStartTime(), type);
+    }
+  }
+
+  /**
+   * Lock information after taking locks.
+   */
+  public static class OmLockObject {
+    private final OmLockInfo omLockInfo;
+    private final List<Lock> locks = new ArrayList<>();
+    private final OmLockStats lockStats = new OmLockStats();

Review Comment:
   No, as this is referred for each request, and can not be static.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks);
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      locks.clear();
+      Thread.currentThread().interrupt();
+      throw new OMException("waiting for locks is interrupted", OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      locks.clear();
+      throw new OMException("Unable to get locks, timeout occurred", OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  /*
+  Optional: If we want more diagnostic info on the type of lock that failed to be acquired (volume, bucket, or key),
+  We can make the parameter a list of objects that wrap the Lock with information about its type.
+  Note that logging the specific volume, bucket or keys this lock was trying to acquire is not helpful and
+  misleading because collisions within the stripe lock might mean we are blocked on a request for a completely
+  different part of the namespace.
+  Obtaining the thread ID that we were waiting on would be more useful, but there is no easy way to do that.
+   */
+  private void acquireLocks(List<Lock> locks) throws TimeoutException, InterruptedException {
+    List<Lock> acquiredLocks = new ArrayList<>(locks.size());
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);
+        } catch (Throwable e) {
+          // We acquired this lock but were unable to add it to our acquired locks list.
+          lock.unlock();
+          releaseLocks(acquiredLocks);
+          throw e;
+        }
+      } else {
+        releaseLocks(acquiredLocks);
+        throw new TimeoutException("Failed to acquire lock after the given timeout.");
+      }
+    }
+  }
+
+  public void unlock(OmLockObject lockObject) {
+    releaseLocks(lockObject.getLocks());
+    lockStatsEnd(lockObject.getLockStats(), lockObject.getLockStatType());
+    lockObject.getLocks().clear();
+  }
+
+  private void releaseLocks(List<Lock> locks) {
+    ListIterator<Lock> reverseIterator = locks.listIterator(locks.size());
+    while (reverseIterator.hasPrevious()) {
+      Lock lock = reverseIterator.previous();
+      lock.unlock();
+    }
+  }
+
+  private static void lockStatsBegin(OmLockStats lockStats, long endTime, long startTime) {
+    lockStats.add(endTime - startTime, OmLockStats.Type.WAIT);
+    lockStats.setLockStartTime(endTime);
+  }
+
+  private static void lockStatsEnd(OmLockStats lockStats, OmLockStats.Type type) {
+    if (lockStats.getLockStartTime() > 0) {
+      lockStats.add(Time.monotonicNowNanos() - lockStats.getLockStartTime(), type);
+    }
+  }
+
+  /**
+   * Lock information after taking locks.
+   */
+  public static class OmLockObject {
+    private final OmLockInfo omLockInfo;
+    private final List<Lock> locks = new ArrayList<>();
+    private final OmLockStats lockStats = new OmLockStats();
+    private OmLockStats.Type lockStatType = OmLockStats.Type.READ;
+
+    public OmLockObject(OmLockInfo lockInfoProvider) {
+      this.omLockInfo = lockInfoProvider;
+    }
+
+    public List<Lock> getLocks() {
+      return locks;
+    }
+
+    public OmLockStats getLockStats() {
+      return lockStats;
+    }
+
+    public OmLockStats.Type getLockStatType() {
+      return lockStatType;
+    }
+
+    public void setLockStatType(OmLockStats.Type lockStatType) {
+      this.lockStatType = lockStatType;
+    }
+
+    public OmLockInfo getOmLockInfo() {
+      return omLockInfo;
+    }
+  }
+
+  /**
+   * lock stats.
+   */
+  public static class OmLockStats {
+    private long lockStartTime;
+    private long waitLockNanos;
+    private long readLockNanos;
+    private long writeLockNanos;
+
+    public long getLockStartTime() {
+      return lockStartTime;
+    }
+
+    public void setLockStartTime(long lockStartTime) {
+      this.lockStartTime = lockStartTime;
+    }
+
+    public long getWaitLockNanos() {
+      return waitLockNanos;
+    }
+
+    public long getReadLockNanos() {
+      return readLockNanos;
+    }
+
+    public long getWriteLockNanos() {
+      return writeLockNanos;
+    }
+
+    void add(long timeNanos, Type type) {

Review Comment:
   done.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "kerneltime (via GitHub)" <gi...@apache.org>.

kerneltime commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2777619792

   > This work to re-architect the locking should probably be performed on a branch and merged to master when it is completed and tested, in order to keep master shipable.
   
   The introduction of locking is not re architecting but adding the capability to serialize in an alternate scheme. Going with feature branch might be too heavy handed for this.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework for obs [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.

swamirishi commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2073384250


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+    List<Lock> locks = new ArrayList<>();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setReadStatsType(false);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks, omLockObject.getLocks());
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new OMException("Waiting for locks is interrupted, " + lockInfo, OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      throw new OMException("Timeout occurred for locks " + lockInfo, OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  private void acquireLocks(List<Lock> locks, Stack<Lock> acquiredLocks) throws TimeoutException, InterruptedException {
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);

Review Comment:
   Use acquiredLocks.push() instead



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+    List<Lock> locks = new ArrayList<>();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setReadStatsType(false);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks, omLockObject.getLocks());
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new OMException("Waiting for locks is interrupted, " + lockInfo, OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      throw new OMException("Timeout occurred for locks " + lockInfo, OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  private void acquireLocks(List<Lock> locks, Stack<Lock> acquiredLocks) throws TimeoutException, InterruptedException {
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);
+        } catch (Throwable e) {
+          // We acquired this lock but were unable to add it to our acquired locks list.
+          lock.unlock();

Review Comment:
   This catch block seems unnecessary as no exception would be thrown



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);

Review Comment:
   Why are number of stripes not configurable?



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2777600523

   @errose28 @szetszwo @swamirishi PR is updated is all review fix.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2052305739


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+    List<Lock> locks = new ArrayList<>();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setReadStatsType(false);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks, omLockObject.getLocks());
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new OMException("Waiting for locks is interrupted, " + lockInfo, OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      throw new OMException("Timeout occurred for locks " + lockInfo, OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  private void acquireLocks(List<Lock> locks, Stack<Lock> acquiredLocks) throws TimeoutException, InterruptedException {

Review Comment:
   added section for having timeout in design doc for lock



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sodonnel (via GitHub)" <gi...@apache.org>.

sodonnel commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2775129243

   This work to re-architect the locking should probably be performed on a branch and merged to master when it is completed and tested, in order to keep master shipable.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2029294980


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmLockInfo.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.lock;
+
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import org.apache.hadoop.ozone.OzoneConsts;
+
+/**
+ * Lock information.
+ */
+public final class OmLockInfo {
+  private final LockInfo volumeLock;
+  private final LockInfo bucketLock;
+  private final Set<LockInfo> keyLocks;
+
+  private OmLockInfo(Builder builder) {
+    volumeLock = builder.volumeLock;
+    bucketLock = builder.bucketLock;
+    keyLocks = builder.keyLocks;
+  }
+
+  public Optional<LockInfo> getVolumeLock() {
+    return Optional.ofNullable(volumeLock);
+  }
+
+  public Optional<LockInfo> getBucketLock() {
+    return Optional.ofNullable(bucketLock);
+  }
+
+  public Optional<Set<LockInfo>> getKeyLocks() {
+    return Optional.ofNullable(keyLocks);
+  }
+
+  @Override
+  public String toString() {
+    StringBuilder sb = new StringBuilder();
+    if (volumeLock != null) {
+      sb.append("Volume:").append(volumeLock);
+    }
+    if (bucketLock != null) {
+      sb.append("Bucket:").append(bucketLock);
+    }
+    if (keyLocks != null) {
+      sb.append("Keys:").append(keyLocks);
+    }
+    return sb.toString();
+  }
+
+  /**
+   * Builds an {@link OmLockInfo} object with optional volume, bucket or key locks.
+   */
+  public static final class Builder {
+    private LockInfo volumeLock;
+    private LockInfo bucketLock;
+    private Set<LockInfo> keyLocks;
+
+    public Builder() {
+    }
+
+    public Builder addVolumeReadLock(String volume) {
+      volumeLock = LockInfo.writeLockInfo(volume);
+      return this;
+    }
+
+    public Builder addVolumeWriteLock(String volume) {
+      volumeLock = LockInfo.readLockInfo(volume);
+      return this;
+    }
+
+    public Builder addBucketReadLock(String volume, String bucket) {
+      bucketLock = LockInfo.readLockInfo(joinStrings(volume, bucket));
+      return this;
+    }
+
+    public Builder addBucketWriteLock(String volume, String bucket) {
+      bucketLock = LockInfo.writeLockInfo(joinStrings(volume, bucket));
+      return this;
+    }
+
+    // Currently there is no use case for key level read locks.
+    public Builder addKeyWriteLock(String volume, String bucket, String key) {
+      // Lazy init keys.
+      if (keyLocks == null) {
+        keyLocks = new HashSet<>();
+      }
+      keyLocks.add(LockInfo.writeLockInfo(joinStrings(volume, bucket, key)));
+      return this;
+    }
+
+    private String joinStrings(String... parts) {
+      return String.join(OzoneConsts.OZONE_URI_DELIMITER, parts);
+    }
+
+    public OmLockInfo build() {
+      return new OmLockInfo(this);
+    }
+  }
+
+  /**
+   * This class provides specifications about a lock's requirements.
+   */
+  public static final class LockInfo implements Comparable<LockInfo> {
+    private final String name;
+    private final boolean isWriteLock;

Review Comment:
   Use an enum is better than a boolean; see ["Never Use Booleans for Something That Has Two States Now, but Might Have More Later"](https://protobuf.dev/best-practices/api/#never-use-booleans-for-two-states).



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+    List<Lock> locks = new ArrayList<>();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setReadStatsType(false);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks, omLockObject.getLocks());
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new OMException("Waiting for locks is interrupted, " + lockInfo, OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      throw new OMException("Timeout occurred for locks " + lockInfo, OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  private void acquireLocks(List<Lock> locks, Stack<Lock> acquiredLocks) throws TimeoutException, InterruptedException {

Review Comment:
   Concurrently, if the `lock.tryLock(..)` is interrupted, the partial lock won't be released.  Getting everything right with timeout is not easy.
   
   If we are adding timeout support, we have to update the design doc first.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.Stack;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+    List<Lock> locks = new ArrayList<>();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setReadStatsType(false);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setReadStatsType(false);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks, omLockObject.getLocks());
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw new OMException("Waiting for locks is interrupted, " + lockInfo, OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      throw new OMException("Timeout occurred for locks " + lockInfo, OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }

Review Comment:
   The [lock overhead](https://en.wikipedia.org/wiki/Lock_(computer_science)#Granularity) probably is quite high in this method.  It may not be able to improve the performance much.  



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2784705743

   UseEpsilonGC (no gc): https://blogs.oracle.com/javamagazine/post/epsilon-the-jdks-do-nothing-garbage-collector
   
   - OmRequestGatekeeper
   ```
   bucketKeyCount     = 1000000
   bucketKeyListCount = 1000
   memory:   167.723 MB (before)
   OmRequestGatekeeper
   time  :     0.552 s
   memory:  1303.723 MB (after)
   memory:  1136.000 MB (diff)
   GC    :         0 ms (0)
   ```
   
   - OmLockManager
   ```
   bucketKeyCount     = 1000000
   bucketKeyListCount = 1000
   memory:   172.629 MB (before)
   OmLockManager
   time  :     0.283 s
   memory:   368.629 MB (after)
   memory:   196.000 MB (diff)
   GC    :         0 ms (0)
   ```


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework for obs [ozone]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] closed pull request #8217: HDDS-12356. granular locking framework for obs
URL: https://github.com/apache/ozone/pull/8217


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2784427590

   Benchmark program for this PR: https://issues.apache.org/jira/secure/attachment/13075848/8217_benchmark.patch


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2028116146


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {

Review Comment:
   Defined OmLockObject as autoclosable, so based on usecase, it can be used with try-with-resouce.
   
   We need return this object as it holds LockStats which is required to be set to HadoopRPC metrics with each request calling this interface.
   This might be added to ozone metrics also to capture lock stats.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.

sumitagrawl commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2777598592

   > This work to re-architect the locking should probably be performed on a branch and merged to master when it is completed and tested, in order to keep master shipable.
   
   This is done incrementally with no impact to existing flow. Original PR is splitted for merge as suggested to @szetszwo for better review and merge back to master in continuation.
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "errose28 (via GitHub)" <gi...@apache.org>.

errose28 commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2784834175

   Some analysis of the benchmark would help, i.e. where is the memory savings coming from? Based on an initial look I think `bucketKeyListCount = 1_000` is skewing the results. Each lock will usually only be taken on 1 key, and sometimes 2. I don't think optimizing on 1k namespace locks at a time is priority, but if there's simple way to get those savings in this PR we can look into it.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "errose28 (via GitHub)" <gi...@apache.org>.

errose28 commented on code in PR #8217:
URL: https://github.com/apache/ozone/pull/8217#discussion_r2024984801


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks);
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      locks.clear();
+      Thread.currentThread().interrupt();
+      throw new OMException("waiting for locks is interrupted", OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      locks.clear();
+      throw new OMException("Unable to get locks, timeout occurred", OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  /*
+  Optional: If we want more diagnostic info on the type of lock that failed to be acquired (volume, bucket, or key),
+  We can make the parameter a list of objects that wrap the Lock with information about its type.
+  Note that logging the specific volume, bucket or keys this lock was trying to acquire is not helpful and
+  misleading because collisions within the stripe lock might mean we are blocked on a request for a completely
+  different part of the namespace.
+  Obtaining the thread ID that we were waiting on would be more useful, but there is no easy way to do that.
+   */
+  private void acquireLocks(List<Lock> locks) throws TimeoutException, InterruptedException {
+    List<Lock> acquiredLocks = new ArrayList<>(locks.size());
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);
+        } catch (Throwable e) {
+          // We acquired this lock but were unable to add it to our acquired locks list.
+          lock.unlock();
+          releaseLocks(acquiredLocks);
+          throw e;
+        }
+      } else {
+        releaseLocks(acquiredLocks);
+        throw new TimeoutException("Failed to acquire lock after the given timeout.");
+      }
+    }
+  }
+
+  public void unlock(OmLockObject lockObject) {
+    releaseLocks(lockObject.getLocks());
+    lockStatsEnd(lockObject.getLockStats(), lockObject.getLockStatType());
+    lockObject.getLocks().clear();
+  }
+
+  private void releaseLocks(List<Lock> locks) {
+    ListIterator<Lock> reverseIterator = locks.listIterator(locks.size());
+    while (reverseIterator.hasPrevious()) {
+      Lock lock = reverseIterator.previous();
+      lock.unlock();
+    }
+  }
+
+  private static void lockStatsBegin(OmLockStats lockStats, long endTime, long startTime) {
+    lockStats.add(endTime - startTime, OmLockStats.Type.WAIT);
+    lockStats.setLockStartTime(endTime);
+  }
+
+  private static void lockStatsEnd(OmLockStats lockStats, OmLockStats.Type type) {
+    if (lockStats.getLockStartTime() > 0) {
+      lockStats.add(Time.monotonicNowNanos() - lockStats.getLockStartTime(), type);
+    }
+  }
+
+  /**
+   * Lock information after taking locks.
+   */
+  public static class OmLockObject {
+    private final OmLockInfo omLockInfo;
+    private final List<Lock> locks = new ArrayList<>();
+    private final OmLockStats lockStats = new OmLockStats();

Review Comment:
   Are these fields intended to be static?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();

Review Comment:
   Nothing initializes the `OmLockObject` lock list now and we already have the locks inside the stripes in this class. Why would we try to get them out of `OmLockObject`, or even add them there in the first place?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {

Review Comment:
   Why return `OmLockObject` and require caller to pass it back to `unlock` instead of using `Autocloseable`?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmRequestGatekeeper.java:
##########
@@ -0,0 +1,242 @@
+/*
+ * 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.lock;
+
+import com.google.common.util.concurrent.Striped;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import org.apache.hadoop.hdds.utils.SimpleStriped;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.util.Time;
+
+/**
+ * Manage locking of volume, bucket, keys and others.
+ */
+public class OmRequestGatekeeper {
+  private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000;
+  private static final int NUM_VOLUME_STRIPES = 1024;
+  private static final int NUM_BUCKET_STRIPES = 1024;
+  private static final int NUM_KEY_STRIPES = 4096;
+
+  private final Striped<ReadWriteLock> volumeLocks;
+  private final Striped<ReadWriteLock> bucketLocks;
+  private final Striped<ReadWriteLock> keyLocks;
+
+  public OmRequestGatekeeper() {
+    volumeLocks = SimpleStriped.readWriteLock(NUM_VOLUME_STRIPES, false);
+    bucketLocks = SimpleStriped.readWriteLock(NUM_BUCKET_STRIPES, false);
+    keyLocks = SimpleStriped.readWriteLock(NUM_KEY_STRIPES, false);
+  }
+
+  public OmLockObject lock(OmLockInfo lockInfo) throws IOException {
+    OmLockObject omLockObject = new OmLockObject(lockInfo);
+    List<Lock> locks = omLockObject.getLocks();
+    long startTime = Time.monotonicNowNanos();
+    Optional<OmLockInfo.LockInfo> optionalVolumeLock = lockInfo.getVolumeLock();
+    Optional<OmLockInfo.LockInfo> optionalBucketLock = lockInfo.getBucketLock();
+    Optional<Set<OmLockInfo.LockInfo>> optionalKeyLocks = lockInfo.getKeyLocks();
+
+    if (optionalVolumeLock.isPresent()) {
+      OmLockInfo.LockInfo volumeLockInfo = optionalVolumeLock.get();
+      if (volumeLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).writeLock());
+      } else {
+        locks.add(volumeLocks.get(volumeLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalBucketLock.isPresent()) {
+      OmLockInfo.LockInfo bucketLockInfo = optionalBucketLock.get();
+      if (bucketLockInfo.isWriteLock()) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).writeLock());
+      } else {
+        locks.add(bucketLocks.get(bucketLockInfo.getName()).readLock());
+      }
+    }
+
+    if (optionalKeyLocks.isPresent()) {
+      for (ReadWriteLock keyLock: keyLocks.bulkGet(optionalKeyLocks.get())) {
+        omLockObject.setLockStatType(OmLockStats.Type.WRITE);
+        locks.add(keyLock.writeLock());
+      }
+    }
+
+    try {
+      acquireLocks(locks);
+      lockStatsBegin(omLockObject.getLockStats(), Time.monotonicNowNanos(), startTime);
+    } catch (InterruptedException e) {
+      locks.clear();
+      Thread.currentThread().interrupt();
+      throw new OMException("waiting for locks is interrupted", OMException.ResultCodes.INTERNAL_ERROR);
+    } catch (TimeoutException e) {
+      locks.clear();
+      throw new OMException("Unable to get locks, timeout occurred", OMException.ResultCodes.TIMEOUT);
+    }
+    return omLockObject;
+  }
+
+  /*
+  Optional: If we want more diagnostic info on the type of lock that failed to be acquired (volume, bucket, or key),
+  We can make the parameter a list of objects that wrap the Lock with information about its type.
+  Note that logging the specific volume, bucket or keys this lock was trying to acquire is not helpful and
+  misleading because collisions within the stripe lock might mean we are blocked on a request for a completely
+  different part of the namespace.
+  Obtaining the thread ID that we were waiting on would be more useful, but there is no easy way to do that.
+   */
+  private void acquireLocks(List<Lock> locks) throws TimeoutException, InterruptedException {
+    List<Lock> acquiredLocks = new ArrayList<>(locks.size());
+    for (Lock lock: locks) {
+      if (lock.tryLock(LOCK_TIMEOUT_DEFAULT, TimeUnit.MILLISECONDS)) {
+        try {
+          acquiredLocks.add(lock);
+        } catch (Throwable e) {
+          // We acquired this lock but were unable to add it to our acquired locks list.
+          lock.unlock();
+          releaseLocks(acquiredLocks);
+          throw e;
+        }
+      } else {
+        releaseLocks(acquiredLocks);
+        throw new TimeoutException("Failed to acquire lock after the given timeout.");
+      }
+    }
+  }
+
+  public void unlock(OmLockObject lockObject) {
+    releaseLocks(lockObject.getLocks());
+    lockStatsEnd(lockObject.getLockStats(), lockObject.getLockStatType());
+    lockObject.getLocks().clear();
+  }
+
+  private void releaseLocks(List<Lock> locks) {
+    ListIterator<Lock> reverseIterator = locks.listIterator(locks.size());
+    while (reverseIterator.hasPrevious()) {
+      Lock lock = reverseIterator.previous();
+      lock.unlock();
+    }
+  }
+
+  private static void lockStatsBegin(OmLockStats lockStats, long endTime, long startTime) {
+    lockStats.add(endTime - startTime, OmLockStats.Type.WAIT);
+    lockStats.setLockStartTime(endTime);
+  }
+
+  private static void lockStatsEnd(OmLockStats lockStats, OmLockStats.Type type) {
+    if (lockStats.getLockStartTime() > 0) {
+      lockStats.add(Time.monotonicNowNanos() - lockStats.getLockStartTime(), type);
+    }
+  }
+
+  /**
+   * Lock information after taking locks.
+   */
+  public static class OmLockObject {
+    private final OmLockInfo omLockInfo;
+    private final List<Lock> locks = new ArrayList<>();
+    private final OmLockStats lockStats = new OmLockStats();
+    private OmLockStats.Type lockStatType = OmLockStats.Type.READ;
+
+    public OmLockObject(OmLockInfo lockInfoProvider) {
+      this.omLockInfo = lockInfoProvider;
+    }
+
+    public List<Lock> getLocks() {
+      return locks;
+    }
+
+    public OmLockStats getLockStats() {
+      return lockStats;
+    }
+
+    public OmLockStats.Type getLockStatType() {
+      return lockStatType;
+    }
+
+    public void setLockStatType(OmLockStats.Type lockStatType) {
+      this.lockStatType = lockStatType;
+    }
+
+    public OmLockInfo getOmLockInfo() {
+      return omLockInfo;
+    }
+  }
+
+  /**
+   * lock stats.
+   */
+  public static class OmLockStats {
+    private long lockStartTime;
+    private long waitLockNanos;
+    private long readLockNanos;
+    private long writeLockNanos;
+
+    public long getLockStartTime() {
+      return lockStartTime;
+    }
+
+    public void setLockStartTime(long lockStartTime) {
+      this.lockStartTime = lockStartTime;
+    }
+
+    public long getWaitLockNanos() {
+      return waitLockNanos;
+    }
+
+    public long getReadLockNanos() {
+      return readLockNanos;
+    }
+
+    public long getWriteLockNanos() {
+      return writeLockNanos;
+    }
+
+    void add(long timeNanos, Type type) {

Review Comment:
   We can remove the `Type` enum all together and just create one method for updating each stat instead of one method to unpack the enum.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.

adoroszlai commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2772747799

   > We are working on prototypes to discuss different approaches
   
   That's fine.  But this is the third PR from @sumitagrawl with basically the same text description.  Having 3 different approaches is OK even from same developer, but please describe properly.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2790458978

   @errose28 , thanks for looking at the benchmark results!
   
   > ... where is the memory savings coming from? ...
   
   The memory consumption is from the multiple unnecessary data structures used in this PR.
   
   > ... bucketKeyListCount = 1_000 is skewing the results. Each lock will usually only be taken on 1 key, and sometimes 2.  ... 
   
   According to @kerneltime and @sumitagrawl , most cases only have 1 key.  That's why I used a smaller bucketKeyListCount compared to bucketKeyCount.  We may try different combinations.  Any suggestions?
   
   
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "errose28 (via GitHub)" <gi...@apache.org>.

errose28 commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2772737745

   @adoroszlai We are working on prototypes to discuss different approaches. I agree it is cluttering the main PR queue and I suggest others to raise the PR against their own fork and link it in a [Jira comment like this](https://issues.apache.org/jira/browse/HDDS-12356?focusedCommentId=17939343&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17939343) instead cc @szetszwo 


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.

adoroszlai commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2772527161

   Why do we need yet another PR?


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2784347746

   Comparing this PR with https://github.com/apache/ozone/pull/8192 :
   - OmRequestGatekeeper (this PR)
   ```
   bucketKeyCount     = 1000000
   bucketKeyListCount = 1000
   memory:   104.336 MB (before)
   OmRequestGatekeeper
   time  :     1.245 s
   memory:   378.625 MB (after)
   memory:   274.289 MB (diff)
   ```
   - OmLockManager (https://github.com/apache/ozone/pull/8192)
   ```
   bucketKeyCount     = 1000000
   bucketKeyListCount = 1000
   memory:   102.547 MB (before)
   OmLockManager
   time  :     0.514 s
   memory:   109.228 MB (after)
   memory:     6.681 MB (diff)
   ```


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework for obs [ozone]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-3549967045

   Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "errose28 (via GitHub)" <gi...@apache.org>.

errose28 commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2779114152

   @sodonnel I am actually in favor of doing this initial set of OBS migration tasks on a feature branch.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "sodonnel (via GitHub)" <gi...@apache.org>.

sodonnel commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2778072930

> The introduction of locking is not re architecting but adding the capability to serialize in an alternate scheme. Going with feature branch might be too heavy handed for this.

@kerneltime Recall the addition of a much smaller change to introduce atomic rewrite, where yourself and @errose28 insisted on doing the development on a branch. That change was much smaller and less impactful than this one.

While I agree this change is "framework code" and will not be impactful to existing logic, the followup tasks might be impactful and done in several PRs if they result in large changes:

```
Next PR will include:

integration of locking framework to flow
locking for obs key operation, bucket operation, volume operation and MPU cases
https://issues.apache.org/jira/browse/HDDS-12386 configuration for lock bucket length and timeout
```

In general I don't agree with pushing development to a branch too quickly, because It is a pain to get the branch merged as it requires a vote. However, we do need consistency on the project and to decide on what requires a branch and what does not. Especially if the goal is to always have "master shippable".

Perhaps the test is, if a PR can leave master in a state where it is not shippable without another PR (eg half the code using the old locking and half using new locking), then the feature should be on a branch, even if the branch is only 3 or 4 commits before merging back to master.

Another question is whether such a branch should need a vote to merge? I'd argue that a short lived branch with a handful of commits is a useful thing and should not require a vote to merge - it encourages small PRs which are easier to review and build up the feature in a more natural way. It would make using branches for this sort of thing light weight and tool to be used rather than a burden.

That all said, I am not involved in this area of work and I am not following how involved or risky this change is overall.

--
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.

szetszwo commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-2773452113

   > That's fine. But this is the third PR from @sumitagrawl with basically the same text description. Having 3 different approaches is OK even from same developer, but please describe properly.
   
   @sumitagrawl , I am also confused for these similar PRs.  We should have a better description on why there are three PRs and what are difference between them, and close the previous PRs if there are no longer needed.
   
   > Based on commits, this seems to be a continuation of https://github.com/apache/ozone/pull/8176. There is absolutely no need to open a new PR for addressing review comments, it just splits the conversation unnecessarily.
   
   Agree.  It is even better to keep using the same PR.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-12356. granular locking framework for obs [ozone]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #8217:
URL: https://github.com/apache/ozone/pull/8217#issuecomment-3519268442

   This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org