[MINOR] refactor: Calling lock() method outside try block to avoid unnecessary errors (#1590)
### What changes were proposed in this pull request?
Calling `lock()` method outside try block to avoid unnecessary errors
### Why are the changes needed?
In general, the `lock()` method should be placed outside the try block. The reason is that if the `lock()` method throws an exception, it indicates that the lock was not acquired, so there is no need to attempt to release it in the finally block. If the `lock()` method is placed within the try block and it throws an exception, the finally block will still be executed and attempt to release a lock that was never acquired, leading to errors.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
diff --git a/client-mr/core/src/main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java b/client-mr/core/src/main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java
index c98870e..1860fe8 100644
--- a/client-mr/core/src/main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java
+++ b/client-mr/core/src/main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java
@@ -297,8 +297,8 @@
} catch (Throwable t) {
LOG.warn("send shuffle data exception ", t);
} finally {
+ memoryLock.lock();
try {
- memoryLock.lock();
if (LOG.isDebugEnabled()) {
LOG.debug("memoryUsedSize {} decrease {}", memoryUsedSize, size);
}
diff --git a/client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssShuffleManager.java b/client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssShuffleManager.java
index 6168876..a734b10 100644
--- a/client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssShuffleManager.java
+++ b/client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssShuffleManager.java
@@ -433,8 +433,8 @@
protected Void callInternal() throws Exception {
long nextReport = 0;
while (!isShutdown.get()) {
+ reportLock.lock();
try {
- reportLock.lock();
while (failedEvents.isEmpty()) {
boolean signaled =
reportCondition.await(maxTimeToWaitForReportMillis, TimeUnit.MILLISECONDS);
@@ -1175,8 +1175,8 @@
srcAttemptIdentifier.getInputIdentifier(),
srcAttemptIdentifier.getAttemptNumber());
if (maxTimeToWaitForReportMillis > 0) {
+ reportLock.lock();
try {
- reportLock.lock();
failedEvents.merge(readError, 1, (a, b) -> a + b);
reportCondition.signal();
} finally {
diff --git a/client-tez/src/main/java/org/apache/tez/runtime/library/common/sort/buffer/WriteBufferManager.java b/client-tez/src/main/java/org/apache/tez/runtime/library/common/sort/buffer/WriteBufferManager.java
index c9a7c47..53cfeba 100644
--- a/client-tez/src/main/java/org/apache/tez/runtime/library/common/sort/buffer/WriteBufferManager.java
+++ b/client-tez/src/main/java/org/apache/tez/runtime/library/common/sort/buffer/WriteBufferManager.java
@@ -281,8 +281,8 @@
} catch (Throwable t) {
LOG.warn("send shuffle data exception ", t);
} finally {
+ memoryLock.lock();
try {
- memoryLock.lock();
if (LOG.isDebugEnabled()) {
LOG.debug("memoryUsedSize {} decrease {}", memoryUsedSize, size);
}
diff --git a/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java b/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
index b26167a..9f82870 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
@@ -267,8 +267,8 @@
ShuffleDataDistributionType dataDistType,
int maxConcurrencyPerPartitionToWrite) {
ReentrantReadWriteLock.WriteLock lock = getAppWriteLock(appId);
+ lock.lock();
try {
- lock.lock();
refreshAppId(appId);
ShuffleTaskInfo taskInfo = shuffleTaskInfos.get(appId);
@@ -753,8 +753,8 @@
@VisibleForTesting
public void removeResources(String appId, boolean checkAppExpired) {
Lock lock = getAppWriteLock(appId);
+ lock.lock();
try {
- lock.lock();
LOG.info("Start remove resource for appId[" + appId + "]");
if (checkAppExpired && !isAppExpired(appId)) {
LOG.info(
