errose28 commented on code in PR #7647: URL: https://github.com/apache/ozone/pull/7647#discussion_r1913513371
########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/KeyLock.java: ########## @@ -0,0 +1,145 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class KeyLock { + private static final Logger LOG = LoggerFactory.getLogger(KeyLock.class); + private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000; Review Comment: Let's make stripe sizes and timeouts configurable. As suggested above though, this would happen in a layer above this class. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/KeyLock.java: ########## @@ -0,0 +1,145 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. Review Comment: Please add a lot more detail to how this class is used ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmLockOpr.java: ########## @@ -0,0 +1,268 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.ThreadFactoryBuilder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import org.apache.hadoop.util.Time; +import org.jheaps.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. Review Comment: Please add much more descriptive javadoc for these critical parts of the system. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java: ########## @@ -190,24 +193,22 @@ private OMResponse internalProcessRequest(OMRequest request) throws ServiceExcep } } - if (!isRatisEnabled()) { - return submitRequestDirectlyToOM(request); - } - if (OmUtils.isReadOnly(request)) { return submitReadRequestToOM(request); } - // To validate credentials we have already verified leader status. - // This will skip of checking leader status again if request has S3Auth. - if (!s3Auth) { - OzoneManagerRatisUtils.checkLeaderStatus(ozoneManager); - } + if (isRatisEnabled()) { Review Comment: This shouldn't be necessary with the current 2.0 changes to remove non-ratis OM from master. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java: ########## @@ -231,16 +232,44 @@ private OMResponse internalProcessRequest(OMRequest request) throws ServiceExcep return createErrorResponse(request, ex); } - final OMResponse response = omRatisServer.submitRequest(requestToSubmit); - if (!response.getSuccess()) { - omClientRequest.handleRequestFailure(ozoneManager); + lockInfo = omClientRequest.lock(ozoneManager, ozoneManager.getOmLockOpr()); + try { + if (isRatisEnabled()) { + final OMResponse response = omRatisServer.submitRequest(requestToSubmit); + if (!response.getSuccess()) { + omClientRequest.handleRequestFailure(ozoneManager); + } + return response; + } else { + return submitRequestDirectlyToOM(request); + } + } finally { + performUnlock(omClientRequest, ozoneManager.getOmLockOpr(), lockInfo); } - return response; + } catch (IOException ex) { + return createErrorResponse(request, ex); } finally { OzoneManager.setS3Auth(null); } } + private static void performUnlock( Review Comment: The design doc mentioned some sort of "GateKeeper" entity, which I was expecting to see in this PR. Currently it looks like locking/unlocking is strewn about multiple places. Can we consolidate coordination to one new `OMRequestGateKeeper` class? ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmLockOpr.java: ########## @@ -0,0 +1,268 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.ThreadFactoryBuilder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import org.apache.hadoop.util.Time; +import org.jheaps.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class OmLockOpr { + private static final Logger LOG = LoggerFactory.getLogger(OmLockOpr.class); + private static final long MONITOR_DELAY = 10 * 60 * 1000; + private static final long MONITOR_LOCK_THRESHOLD_NS = 10 * 60 * 1000_000_000L; + private final KeyLock keyLocking; + private final KeyLock bucketLocking; + private final KeyLock volumeLocking; + private final String threadNamePrefix; + private ScheduledExecutorService executorService; + private final Map<OmLockInfo, OmLockInfo> lockMonitorMap = new ConcurrentHashMap<>(); + + public OmLockOpr(String threadNamePrefix) { + this.threadNamePrefix = threadNamePrefix; + keyLocking = new KeyLock(102400); + bucketLocking = new KeyLock(1024); + volumeLocking = new KeyLock(1024); Review Comment: This is the layer that should be reading config keys and passing them into the `KeyLock` class. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/KeyLock.java: ########## @@ -0,0 +1,145 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class KeyLock { + private static final Logger LOG = LoggerFactory.getLogger(KeyLock.class); + private static final long LOCK_TIMEOUT_DEFAULT = 10 * 60 * 1000; + private final Striped<ReadWriteLock> fileStripedLock; + private final long lockTimeout; + + public KeyLock(int stripLockSize) { + this(stripLockSize, LOCK_TIMEOUT_DEFAULT); + } + + public KeyLock(int stripLockSize, long timeout) { + fileStripedLock = Striped.readWriteLock(stripLockSize); + lockTimeout = timeout; + } + + public List<Lock> lock(List<String> keyList) throws IOException { + List<Lock> locks = new ArrayList<>(); + boolean isSuccess = false; + try { + Iterable<ReadWriteLock> readWriteLocks = fileStripedLock.bulkGet(keyList); + for (ReadWriteLock rwLock : readWriteLocks) { + Lock lockObj = rwLock.writeLock(); + boolean b = lockObj.tryLock(lockTimeout, TimeUnit.MILLISECONDS); + if (!b) { + LOG.error("Key write lock is failed for {} after wait of {}ms", this, lockTimeout); + throw new OMException("Unable to get write lock after " + lockTimeout + "ms" + + ", read lock info: " + rwLock.readLock(), + OMException.ResultCodes.TIMEOUT); + } + locks.add(lockObj); + } + isSuccess = true; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new OMException("Unable to get write lock as interrupted", OMException.ResultCodes.INTERNAL_ERROR); + } finally { + if (!isSuccess) { + Collections.reverse(locks); + locks.forEach(Lock::unlock); + locks.clear(); + } + } + return locks; + } + + public Lock lock(String key) throws IOException { + LOG.debug("Key {} is locked for instance {} {}", key, this, fileStripedLock.get(key)); + try { + Lock lockObj = fileStripedLock.get(key).writeLock(); + boolean b = lockObj.tryLock(lockTimeout, TimeUnit.MILLISECONDS); + if (!b) { + LOG.error("Key {} lock is failed for {} after wait of {}ms", key, this, lockTimeout); + throw new OMException("Unable to get write lock for " + key + " after " + lockTimeout + "ms" + + ", read lock info: " + fileStripedLock.get(key).readLock(), + OMException.ResultCodes.TIMEOUT); + } + return lockObj; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new OMException("Unable to get read lock for " + key + " is interrupted", + OMException.ResultCodes.INTERNAL_ERROR); + } + } + + public List<Lock> readLock(List<String> keyList) throws OMException { Review Comment: Why do we need read lock? In the new flow RocksDB should handle consistent views without a cache. Is this just for compatibility with the old flow? ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java: ########## @@ -121,6 +122,18 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { } + public OmLockOpr.OmLockInfo lock(OzoneManager ozoneManager, OmLockOpr lockOpr) throws IOException { Review Comment: Why are we touching FSO/Legacy requests in this OBS patch? ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmLockOpr.java: ########## @@ -0,0 +1,268 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.ThreadFactoryBuilder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import org.apache.hadoop.util.Time; +import org.jheaps.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class OmLockOpr { + private static final Logger LOG = LoggerFactory.getLogger(OmLockOpr.class); + private static final long MONITOR_DELAY = 10 * 60 * 1000; + private static final long MONITOR_LOCK_THRESHOLD_NS = 10 * 60 * 1000_000_000L; + private final KeyLock keyLocking; + private final KeyLock bucketLocking; + private final KeyLock volumeLocking; + private final String threadNamePrefix; + private ScheduledExecutorService executorService; + private final Map<OmLockInfo, OmLockInfo> lockMonitorMap = new ConcurrentHashMap<>(); + + public OmLockOpr(String threadNamePrefix) { + this.threadNamePrefix = threadNamePrefix; + keyLocking = new KeyLock(102400); + bucketLocking = new KeyLock(1024); + volumeLocking = new KeyLock(1024); + } + + public void start() { + ThreadFactory threadFactory = new ThreadFactoryBuilder().setDaemon(true) Review Comment: We can extend the existing `BackgroundService` class to keep all background service management consistent ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java: ########## @@ -122,6 +123,13 @@ public OMRequest preExecute(OzoneManager ozoneManager) return omRequest; } + public OmLockOpr.OmLockInfo lock(OzoneManager ozoneManager, OmLockOpr lockOpr) throws IOException { + return null; + } + + public void unlock(OmLockOpr lockOpr, OmLockOpr.OmLockInfo lockInfo) { + } Review Comment: What is the purpose of defining these stubs in the parent class? I don't see why anyone would call them from outside (public) or using the generic `OMClientRequest` type. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/OmLockOpr.java: ########## @@ -0,0 +1,268 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.ThreadFactoryBuilder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import org.apache.hadoop.util.Time; +import org.jheaps.annotations.VisibleForTesting; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class OmLockOpr { + private static final Logger LOG = LoggerFactory.getLogger(OmLockOpr.class); + private static final long MONITOR_DELAY = 10 * 60 * 1000; + private static final long MONITOR_LOCK_THRESHOLD_NS = 10 * 60 * 1000_000_000L; + private final KeyLock keyLocking; + private final KeyLock bucketLocking; + private final KeyLock volumeLocking; + private final String threadNamePrefix; + private ScheduledExecutorService executorService; + private final Map<OmLockInfo, OmLockInfo> lockMonitorMap = new ConcurrentHashMap<>(); + + public OmLockOpr(String threadNamePrefix) { + this.threadNamePrefix = threadNamePrefix; + keyLocking = new KeyLock(102400); + bucketLocking = new KeyLock(1024); + volumeLocking = new KeyLock(1024); + } + + public void start() { + ThreadFactory threadFactory = new ThreadFactoryBuilder().setDaemon(true) + .setNameFormat(threadNamePrefix + "OmLockOpr-Monitor-%d").build(); + executorService = Executors.newScheduledThreadPool(1, threadFactory); + executorService.scheduleWithFixedDelay(this::monitor, 0, MONITOR_DELAY, TimeUnit.MILLISECONDS); + } + + public void stop() { + executorService.shutdown(); + } + + public OmLockInfo volumeReadLock(String volumeName) throws IOException { Review Comment: We should try to greatly reduce the API surface required here. My understanding is that only one lock used at write time for a volume, bucket, or key is required, but the design doc unfortunately does not provide more info. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/KeyLock.java: ########## @@ -0,0 +1,145 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * key locking. + */ +public class KeyLock { Review Comment: I think this name is confusing. It seems to imply that it for locking "keys" as they are defined in the Ozone namespace. It is actually a generic class that supports locking based on any string, regardless of its significance to the system. I would move this to one of the common modules outside of OM, have it take any configurations as parameters, and throw general typed exceptions that upper layers can chain on to specific service type exceptions like `OMException`. This class seems to fit in the same family of utils as [SimpleStripe](https://github.com/apache/ozone/blob/af72296b0a2722b69e104aae7c5f8dcdab438cc0/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/SimpleStriped.java#L42). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
