[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295956276
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -154,79 +178,137 @@ public void releaseVolumeLock(String volume) {
}
/**
- * Acquires S3 Bucket lock on the given resource.
+ * Acquires bucket lock on the given resource.
*
* If the lock is not available then the current thread becomes
- * disabled for thread scheduling purposes and lies dormant until the lock
has
- * been acquired.
+ * disabled for thread scheduling purposes and lies dormant until the
+ * lock has been acquired.
*
- * @param s3BucketName S3Bucket Name on which the lock has to be acquired
+ * @param bucket Bucket on which the lock has to be acquired
*/
- public void acquireS3Lock(String s3BucketName) {
-// Calling thread should not hold any bucket lock.
-// You can take an Volume while holding S3 bucket lock, since
-// semantically an S3 bucket maps to the ozone volume. So we check here
-// only if ozone bucket lock is taken.
-if (hasAnyBucketLock()) {
+ public void acquireBucketLock(String volume, String bucket) {
+if (hasAnyUserLock()) {
throw new RuntimeException(
"Thread '" + Thread.currentThread().getName() +
- "' cannot acquire S3 bucket lock while holding Ozone bucket " +
- "lock(s).");
+ "' cannot acquire bucket lock while holding User lock.");
}
-manager.lock(OM_S3_PREFIX + s3BucketName);
-myLocks.get().get(S3_BUCKET_LOCK).incrementAndGet();
+manager.lock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
+myLocks.get().get(BUCKET_LOCK).incrementAndGet();
}
/**
- * Releases the volume lock on given resource.
+ * Releases the bucket lock on given resource.
*/
- public void releaseS3Lock(String s3BucketName) {
-manager.unlock(OM_S3_PREFIX + s3BucketName);
-myLocks.get().get(S3_BUCKET_LOCK).decrementAndGet();
+ public void releaseBucketLock(String volume, String bucket) {
+manager.unlock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
+myLocks.get().get(BUCKET_LOCK).decrementAndGet();
}
/**
- * Acquires bucket lock on the given resource.
+ * Acquires user lock on the given resource.
*
* If the lock is not available then the current thread becomes
* disabled for thread scheduling purposes and lies dormant until the
* lock has been acquired.
*
- * @param bucket Bucket on which the lock has to be acquired
+ * @param user User on which the lock has to be acquired
*/
- public void acquireBucketLock(String volume, String bucket) {
-manager.lock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
-myLocks.get().get(BUCKET_LOCK).incrementAndGet();
+ public void acquireUserLock(String user) {
+// In order to not maintain username's on which we have acquired lock,
+// just checking have we acquired userLock before. If user want's to
+// acquire user lock on multiple user's they should use
+// acquireMultiUserLock. This is just a protection logic, to let not users
+// use this if acquiring lock on multiple users. As currently, we have only
+// use case we have for this is during setOwner operation in VolumeManager.
+if (hasAnyUserLock()) {
+ LOG.error("Already have userLock");
+ throw new RuntimeException("For acquiring lock on multiple users, use " +
+ "acquireMultiLock method");
+}
+manager.lock(OM_USER_PREFIX + user);
+myLocks.get().get(USER_LOCK).incrementAndGet();
}
/**
- * Releases the bucket lock on given resource.
+ * Releases the user lock on given resource.
*/
- public void releaseBucketLock(String volume, String bucket) {
-manager.unlock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
-myLocks.get().get(BUCKET_LOCK).decrementAndGet();
+ public void releaseUserLock(String user) {
+manager.unlock(OM_USER_PREFIX + user);
+myLocks.get().get(USER_LOCK).decrementAndGet();
}
/**
- * Returns true if the current thread holds any volume lock.
- * @return true if current thread holds volume lock, else false
+ * Acquire user lock on 2 users. In this case, we compare 2 strings
+ * lexicographically, and acquire the locks according to the sorted order of
+ * the user names. In this way, when acquiring locks on multiple user's, we
+ * can avoid dead locks. This method should be called when single thread is
+ * acquiring lock on 2 users at a time.
+ *
+ * Example:
+ * ozone, hdfs -> lock acquire order will be hdfs, ozone
+ * hdfs, ozone -> lock acquire order will be hdfs, ozone
+ *
+ * @param newUser
+ * @param oldUser
*/
- private boolean hasAnyVolumeLock() {
-return myLocks.get().get(VOLUME_LOCK).get() != 0;
+ public void
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295955274
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -132,14 +157,13 @@ public void releaseUserLock(String user) {
* @param volume Volume on which the lock has to be acquired
*/
public void acquireVolumeLock(String volume) {
-// Calling thread should not hold any bucket lock.
+// Calling thread should not hold any bucket/user lock.
// You can take an Volume while holding S3 bucket lock, since
-// semantically an S3 bucket maps to the ozone volume. So we check here
-// only if ozone bucket lock is taken.
-if (hasAnyBucketLock()) {
+// semantically an S3 bucket maps to the ozone volume.
+if (hasAnyBucketLock() || hasAnyUserLock()) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295955075
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -96,30 +112,39 @@ public OzoneManagerLock(Configuration conf) {
}
/**
- * Acquires user lock on the given resource.
+ * Acquires S3 Bucket lock on the given resource.
*
* If the lock is not available then the current thread becomes
- * disabled for thread scheduling purposes and lies dormant until the
- * lock has been acquired.
+ * disabled for thread scheduling purposes and lies dormant until the lock
has
+ * been acquired.
*
- * @param user User on which the lock has to be acquired
+ * @param s3BucketName S3Bucket Name on which the lock has to be acquired
*/
- public void acquireUserLock(String user) {
-// Calling thread should not hold any volume or bucket lock.
-if (hasAnyVolumeLock() || hasAnyBucketLock() || hasAnyS3Lock()) {
+ public void acquireS3BucketLock(String s3BucketName) {
+// Calling thread should not hold any volume/bucket/user lock.
+
+// Not added checks for prefix/s3 secret lock, as they will never be
+// taken with s3Bucket Lock. In this way, we can avoid 2 checks every
+// time we acquire s3Bucket lock.
+
+// Or do we need to add this for future safe?
+
+if (hasAnyVolumeLock() || hasAnyBucketLock() || hasAnyUserLock()) {
throw new RuntimeException(
"Thread '" + Thread.currentThread().getName() +
- "' cannot acquire user lock" +
- " while holding volume, bucket or S3 bucket lock(s).");
+ "' cannot acquire S3 bucket lock while holding Ozone " +
+ "Volume/Bucket/User lock(s).");
}
-manager.lock(OM_USER_PREFIX + user);
+manager.lock(OM_S3_PREFIX + s3BucketName);
Review comment:
This will be done in a new jira.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295954991
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -96,30 +112,39 @@ public OzoneManagerLock(Configuration conf) {
}
/**
- * Acquires user lock on the given resource.
+ * Acquires S3 Bucket lock on the given resource.
*
* If the lock is not available then the current thread becomes
- * disabled for thread scheduling purposes and lies dormant until the
- * lock has been acquired.
+ * disabled for thread scheduling purposes and lies dormant until the lock
has
+ * been acquired.
*
- * @param user User on which the lock has to be acquired
+ * @param s3BucketName S3Bucket Name on which the lock has to be acquired
*/
- public void acquireUserLock(String user) {
-// Calling thread should not hold any volume or bucket lock.
-if (hasAnyVolumeLock() || hasAnyBucketLock() || hasAnyS3Lock()) {
+ public void acquireS3BucketLock(String s3BucketName) {
+// Calling thread should not hold any volume/bucket/user lock.
+
+// Not added checks for prefix/s3 secret lock, as they will never be
+// taken with s3Bucket Lock. In this way, we can avoid 2 checks every
+// time we acquire s3Bucket lock.
+
+// Or do we need to add this for future safe?
+
+if (hasAnyVolumeLock() || hasAnyBucketLock() || hasAnyUserLock()) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295954759
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -59,32 +68,39 @@
*
* {@literal ->} acquireVolumeLock (will work)
* {@literal +->} acquireBucketLock (will work)
- * {@literal +-->} acquireUserLock (will throw Exception)
+ * {@literal +-->} acquireS3BucketLock (will throw Exception)
*
*
- * To acquire a user lock you should not hold any Volume/Bucket lock. Similarly
- * to acquire a Volume lock you should not hold any Bucket lock.
+ * To acquire a S3 lock you should not hold any Volume/Bucket lock. Similarly
+ * to acquire a Volume lock you should not hold any Bucket/User/S3
+ * Secret/Prefix lock.
*/
public final class OzoneManagerLock {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(OzoneManagerLock.class);
+
+ private static final String S3_BUCKET_LOCK = "s3BucketLock";
private static final String VOLUME_LOCK = "volumeLock";
private static final String BUCKET_LOCK = "bucketLock";
- private static final String PREFIX_LOCK = "prefixLock";
- private static final String S3_BUCKET_LOCK = "s3BucketLock";
+ private static final String USER_LOCK = "userLock";
private static final String S3_SECRET_LOCK = "s3SecretetLock";
+ private static final String PREFIX_LOCK = "prefixLock";
+
private final LockManager manager;
// To maintain locks held by current thread.
private final ThreadLocal> myLocks =
ThreadLocal.withInitial(
- () -> ImmutableMap.of(
- VOLUME_LOCK, new AtomicInteger(0),
- BUCKET_LOCK, new AtomicInteger(0),
- PREFIX_LOCK, new AtomicInteger(0),
- S3_BUCKET_LOCK, new AtomicInteger(0),
- S3_SECRET_LOCK, new AtomicInteger(0)
- )
+ () -> ImmutableMap.builder()
+ .put(S3_BUCKET_LOCK, new AtomicInteger(0))
+ .put(VOLUME_LOCK, new AtomicInteger(0))
+ .put(BUCKET_LOCK, new AtomicInteger(0))
+ .put(USER_LOCK, new AtomicInteger(0))
+ .put(S3_SECRET_LOCK, new AtomicInteger(0))
+ .put(PREFIX_LOCK, new AtomicInteger(0))
+ .build()
Review comment:
This will be taken up in a new jira.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295953686
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -154,79 +178,137 @@ public void releaseVolumeLock(String volume) {
}
/**
- * Acquires S3 Bucket lock on the given resource.
+ * Acquires bucket lock on the given resource.
*
* If the lock is not available then the current thread becomes
- * disabled for thread scheduling purposes and lies dormant until the lock
has
- * been acquired.
+ * disabled for thread scheduling purposes and lies dormant until the
+ * lock has been acquired.
*
- * @param s3BucketName S3Bucket Name on which the lock has to be acquired
+ * @param bucket Bucket on which the lock has to be acquired
*/
- public void acquireS3Lock(String s3BucketName) {
-// Calling thread should not hold any bucket lock.
-// You can take an Volume while holding S3 bucket lock, since
-// semantically an S3 bucket maps to the ozone volume. So we check here
-// only if ozone bucket lock is taken.
-if (hasAnyBucketLock()) {
+ public void acquireBucketLock(String volume, String bucket) {
+if (hasAnyUserLock()) {
throw new RuntimeException(
"Thread '" + Thread.currentThread().getName() +
- "' cannot acquire S3 bucket lock while holding Ozone bucket " +
- "lock(s).");
+ "' cannot acquire bucket lock while holding User lock.");
}
-manager.lock(OM_S3_PREFIX + s3BucketName);
-myLocks.get().get(S3_BUCKET_LOCK).incrementAndGet();
+manager.lock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
+myLocks.get().get(BUCKET_LOCK).incrementAndGet();
}
/**
- * Releases the volume lock on given resource.
+ * Releases the bucket lock on given resource.
*/
- public void releaseS3Lock(String s3BucketName) {
-manager.unlock(OM_S3_PREFIX + s3BucketName);
-myLocks.get().get(S3_BUCKET_LOCK).decrementAndGet();
+ public void releaseBucketLock(String volume, String bucket) {
+manager.unlock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
+myLocks.get().get(BUCKET_LOCK).decrementAndGet();
}
/**
- * Acquires bucket lock on the given resource.
+ * Acquires user lock on the given resource.
*
* If the lock is not available then the current thread becomes
* disabled for thread scheduling purposes and lies dormant until the
* lock has been acquired.
*
- * @param bucket Bucket on which the lock has to be acquired
+ * @param user User on which the lock has to be acquired
*/
- public void acquireBucketLock(String volume, String bucket) {
-manager.lock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
-myLocks.get().get(BUCKET_LOCK).incrementAndGet();
+ public void acquireUserLock(String user) {
+// In order to not maintain username's on which we have acquired lock,
+// just checking have we acquired userLock before. If user want's to
+// acquire user lock on multiple user's they should use
+// acquireMultiUserLock. This is just a protection logic, to let not users
+// use this if acquiring lock on multiple users. As currently, we have only
+// use case we have for this is during setOwner operation in VolumeManager.
+if (hasAnyUserLock()) {
+ LOG.error("Already have userLock");
+ throw new RuntimeException("For acquiring lock on multiple users, use " +
+ "acquireMultiLock method");
+}
+manager.lock(OM_USER_PREFIX + user);
+myLocks.get().get(USER_LOCK).incrementAndGet();
}
/**
- * Releases the bucket lock on given resource.
+ * Releases the user lock on given resource.
*/
- public void releaseBucketLock(String volume, String bucket) {
-manager.unlock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
-myLocks.get().get(BUCKET_LOCK).decrementAndGet();
+ public void releaseUserLock(String user) {
+manager.unlock(OM_USER_PREFIX + user);
+myLocks.get().get(USER_LOCK).decrementAndGet();
}
/**
- * Returns true if the current thread holds any volume lock.
- * @return true if current thread holds volume lock, else false
+ * Acquire user lock on 2 users. In this case, we compare 2 strings
+ * lexicographically, and acquire the locks according to the sorted order of
+ * the user names. In this way, when acquiring locks on multiple user's, we
+ * can avoid dead locks. This method should be called when single thread is
+ * acquiring lock on 2 users at a time.
+ *
+ * Example:
+ * ozone, hdfs -> lock acquire order will be hdfs, ozone
+ * hdfs, ozone -> lock acquire order will be hdfs, ozone
+ *
+ * @param newUser
+ * @param oldUser
*/
- private boolean hasAnyVolumeLock() {
-return myLocks.get().get(VOLUME_LOCK).get() != 0;
+ public void
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295952709
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -258,12 +348,61 @@ public void acquirePrefixLock(String prefixPath) {
myLocks.get().get(PREFIX_LOCK).incrementAndGet();
}
- private boolean hasAnyPrefixLock() {
-return myLocks.get().get(PREFIX_LOCK).get() != 0;
- }
-
+ /**
+ * Releases the prefix lock on given resource.
+ */
public void releasePrefixLock(String prefixPath) {
manager.unlock(prefixPath);
myLocks.get().get(PREFIX_LOCK).decrementAndGet();
}
+
+ /**
+ * Returns true if the current thread holds any volume lock.
+ * @return true if current thread holds volume lock, else false
+ */
+ private boolean hasAnyVolumeLock() {
+return myLocks.get().get(VOLUME_LOCK).get() != 0;
+ }
+
+ /**
+ * Returns true if the current thread holds any bucket lock.
+ * @return true if current thread holds bucket lock, else false
+ */
+ private boolean hasAnyBucketLock() {
+return myLocks.get().get(BUCKET_LOCK).get() != 0;
+ }
+
+ /**
+ * Returns true if the current thread holds any s3 bucket lock.
+ * @return true if current thread holds s3 bucket lock, else false
+ */
+ private boolean hasAnyS3BucketLock() {
+return myLocks.get().get(S3_BUCKET_LOCK).get() != 0;
+ }
+
+ /**
+ * Returns true if the current thread holds any user lock.
+ * @return true if current thread holds user lock, else false
+ */
+ private boolean hasAnyUserLock() {
+return myLocks.get().get(USER_LOCK).get() != 0;
Review comment:
Yes, added a call for hasAnyUserLock() in acquireUserLock() so that if some
one is trying to acquire multiple user locks, he will immediately fail with
RunTimeException. As said in code comments in acquireUserLock() this is a
protection logic, in avoiding users do that.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295951701
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
##
@@ -154,79 +178,137 @@ public void releaseVolumeLock(String volume) {
}
/**
- * Acquires S3 Bucket lock on the given resource.
+ * Acquires bucket lock on the given resource.
*
* If the lock is not available then the current thread becomes
- * disabled for thread scheduling purposes and lies dormant until the lock
has
- * been acquired.
+ * disabled for thread scheduling purposes and lies dormant until the
+ * lock has been acquired.
*
- * @param s3BucketName S3Bucket Name on which the lock has to be acquired
+ * @param bucket Bucket on which the lock has to be acquired
*/
- public void acquireS3Lock(String s3BucketName) {
-// Calling thread should not hold any bucket lock.
-// You can take an Volume while holding S3 bucket lock, since
-// semantically an S3 bucket maps to the ozone volume. So we check here
-// only if ozone bucket lock is taken.
-if (hasAnyBucketLock()) {
+ public void acquireBucketLock(String volume, String bucket) {
+if (hasAnyUserLock()) {
throw new RuntimeException(
"Thread '" + Thread.currentThread().getName() +
- "' cannot acquire S3 bucket lock while holding Ozone bucket " +
- "lock(s).");
+ "' cannot acquire bucket lock while holding User lock.");
}
-manager.lock(OM_S3_PREFIX + s3BucketName);
-myLocks.get().get(S3_BUCKET_LOCK).incrementAndGet();
+manager.lock(OM_KEY_PREFIX + volume + OM_KEY_PREFIX + bucket);
Review comment:
Yes, we prefix volumeName with /(OM_KEY_PREFIX) and bucketName with
/(OM_KEY_PREFIX)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295948310
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java
##
@@ -104,37 +104,23 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
OmVolumeArgs omVolumeArgs = null;
String owner = null;
-
+IOException exception = null;
+OzoneManagerProtocolProtos.VolumeList newVolumeList = null;
omMetadataManager.getLock().acquireVolumeLock(volume);
try {
owner = getVolumeInfo(omMetadataManager, volume).getOwnerName();
-} catch (IOException ex) {
- LOG.error("Volume deletion failed for volume:{}", volume, ex);
- omMetrics.incNumVolumeDeleteFails();
- auditLog(auditLogger, buildAuditMessage(OMAction.DELETE_VOLUME,
- buildVolumeAuditMap(volume), ex, userInfo));
- return new OMVolumeDeleteResponse(null, null, null,
- createErrorOMResponse(omResponse, ex));
-} finally {
- omMetadataManager.getLock().releaseVolumeLock(volume);
-}
-// Release and reacquire lock for now it will not be a problem for now, as
-// applyTransaction serializes the operation's.
-// TODO: Revisit this logic once HDDS-1672 checks in.
+ // Release and reacquire lock for now it will not be a problem for now,
as
+ // applyTransaction serializes the operation's.
-// We cannot acquire user lock holding volume lock, so released volume
-// lock, and acquiring user and volume lock.
+ // We cannot acquire user lock holding volume lock, so released volume
+ // lock, and acquiring user and volume lock.
-omMetadataManager.getLock().acquireUserLock(owner);
-omMetadataManager.getLock().acquireVolumeLock(volume);
+ omMetadataManager.getLock().acquireUserLock(owner);
Review comment:
That is why we checked in finally owner!=null and then only release the lock
in finally.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295947682
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3BucketManagerImpl.java
##
@@ -101,34 +101,26 @@ public void createS3Bucket(String userName, String
bucketName)
// anonymous access to bucket where the user name is absent.
String ozoneVolumeName = formatOzoneVolumeName(userName);
-omMetadataManager.getLock().acquireS3Lock(bucketName);
-try {
- String bucket =
- omMetadataManager.getS3Table().get(bucketName);
-
- if (bucket != null) {
-LOG.debug("Bucket already exists. {}", bucketName);
-throw new OMException(
-"Unable to create S3 bucket. " + bucketName + " already exists.",
-OMException.ResultCodes.S3_BUCKET_ALREADY_EXISTS);
- }
- String ozoneBucketName = bucketName;
- createOzoneBucket(ozoneVolumeName, ozoneBucketName);
- String finalName = String.format("%s/%s", ozoneVolumeName,
- ozoneBucketName);
+String bucket = omMetadataManager.getS3Table().get(bucketName);
- omMetadataManager.getS3Table().put(bucketName, finalName);
-} finally {
- omMetadataManager.getLock().releaseS3Lock(bucketName);
Review comment:
Do you mean s3 bucket lock, as we need to acquire that before creating
volume. So, that is acquired in caller in OzoneManager.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295946812
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##
@@ -2564,6 +2564,9 @@ public void createS3Bucket(String userName, String
s3BucketName)
}
metrics.incNumBucketCreates();
try {
+metadataManager.getLock().acquireS3BucketLock(s3BucketName);
+metadataManager.getLock().acquireVolumeLock(
Review comment:
On a side note: once we use new HA code this will be cleaned up, so not
considered much refactoring here.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve locking in OzoneManager.
bharatviswa504 commented on a change in pull request #949: HDDS-1672. Improve
locking in OzoneManager.
URL: https://github.com/apache/hadoop/pull/949#discussion_r295946327
##
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##
@@ -2564,6 +2564,9 @@ public void createS3Bucket(String userName, String
s3BucketName)
}
metrics.incNumBucketCreates();
try {
+metadataManager.getLock().acquireS3BucketLock(s3BucketName);
+metadataManager.getLock().acquireVolumeLock(
Review comment:
I see the only case for failing is with RunTimeException. So, do we still
need the flags?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
