This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
The following commit(s) were added to refs/heads/master by this push:
new 32d533d22 [MINOR] refactor: Calling lock() method outside try block to
avoid unnecessary errors (#1590)
32d533d22 is described below
commit 32d533d226346600f9ed945c81bc19bf0c7c2994
Author: RickyMa <[email protected]>
AuthorDate: Thu Mar 21 11:35:20 2024 +0800
[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.
---
.../main/java/org/apache/hadoop/mapred/SortWriteBufferManager.java | 2 +-
.../tez/runtime/library/common/shuffle/impl/RssShuffleManager.java | 4 ++--
.../tez/runtime/library/common/sort/buffer/WriteBufferManager.java | 2 +-
.../src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
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 c98870e99..1860fe856 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 @@ public class SortWriteBufferManager<K, V> {
} 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 61688768d..a734b102b 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 @@ public class RssShuffleManager extends ShuffleManager {
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 @@ public class RssShuffleManager extends ShuffleManager {
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 c9a7c474a..53cfeba45 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 @@ public class WriteBufferManager<K, V> {
} 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 b26167a2f..9f82870da 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 @@ public class ShuffleTaskManager {
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 @@ public class ShuffleTaskManager {
@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(
