[incubator-uniffle] branch master updated: Cleanup RuntimeException and fetchRemoteStorage logic in ClientUtils (#295)

[ro...@apache.org]

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 6b65bbb2 Cleanup RuntimeException and fetchRemoteStorage logic in ClientUtils (#295)
6b65bbb2 is described below

commit 6b65bbb2a227ef39eef98c9c4b1632ff4e624534
Author: Kaijie Chen <ck...@apache.org>
AuthorDate: Fri Nov 11 10:07:58 2022 +0800

    Cleanup RuntimeException and fetchRemoteStorage logic in ClientUtils (#295)
    
    ### What changes were proposed in this pull request?
    
    1. Replace raw `RuntimeException` with proper specific subclass of `RuntimeException`.
    2. Cleanup the logic in `ClientUtils#fetchRemoteStorage()`.
    
    ### Why are the changes needed?
    
    1. Throwing `RuntimeException` is not a good practice because it's difficult to catch (without catching other RE).
    2. For clearity and easier to understand.
    
    ### Does this PR introduce _any_ user-facing change?
    
    No.
    
    ### How was this patch tested?
    
    ClientUtilsTest is modified to assert more specific exceptions.
---
 .../apache/uniffle/client/util/ClientUtils.java    | 20 +++++---------
 .../org/apache/uniffle/client/ClientUtilsTest.java | 31 +++++++---------------
 2 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java b/client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java
index 26adc794..3c394d23 100644
--- a/client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java
+++ b/client/src/main/java/org/apache/uniffle/client/util/ClientUtils.java
@@ -30,15 +30,15 @@ public class ClientUtils {
   // taskAttemptId is rest of 20 bit, max value is 2^20 - 1
   public static Long getBlockId(long partitionId, long taskAttemptId, long atomicInt) {
     if (atomicInt < 0 || atomicInt > Constants.MAX_SEQUENCE_NO) {
-      throw new RuntimeException("Can't support sequence[" + atomicInt
+      throw new IllegalArgumentException("Can't support sequence[" + atomicInt
           + "], the max value should be " + Constants.MAX_SEQUENCE_NO);
     }
     if (partitionId < 0 || partitionId > Constants.MAX_PARTITION_ID) {
-      throw new RuntimeException("Can't support partitionId["
+      throw new IllegalArgumentException("Can't support partitionId["
           + partitionId + "], the max value should be " + Constants.MAX_PARTITION_ID);
     }
     if (taskAttemptId < 0 || taskAttemptId > Constants.MAX_TASK_ATTEMPT_ID) {
-      throw new RuntimeException("Can't support taskAttemptId["
+      throw new IllegalArgumentException("Can't support taskAttemptId["
           + taskAttemptId + "], the max value should be " + Constants.MAX_TASK_ATTEMPT_ID);
     }
     return (atomicInt << (Constants.PARTITION_ID_MAX_LENGTH + Constants.TASK_ATTEMPT_ID_MAX_LENGTH))
@@ -52,19 +52,13 @@ public class ClientUtils {
       String storageType,
       ShuffleWriteClient shuffleWriteClient) {
     RemoteStorageInfo remoteStorage = defaultRemoteStorage;
-    if (remoteStorage.isEmpty() && requireRemoteStorage(storageType)) {
-      if (dynamicConfEnabled) {
-        // get from coordinator first
+    if (requireRemoteStorage(storageType)) {
+      if (remoteStorage.isEmpty() && dynamicConfEnabled) {
+        // fallback to dynamic conf on coordinator
         remoteStorage = shuffleWriteClient.fetchRemoteStorage(appId);
-        if (remoteStorage.isEmpty()) {
-          // empty from coordinator, use default remote storage
-          remoteStorage = defaultRemoteStorage;
-        }
-      } else {
-        remoteStorage = defaultRemoteStorage;
       }
       if (remoteStorage.isEmpty()) {
-        throw new RuntimeException("Can't find remoteStorage: with storageType[" + storageType + "]");
+        throw new IllegalStateException("Can't find remoteStorage: with storageType[" + storageType + "]");
       }
     }
     return remoteStorage;
diff --git a/client/src/test/java/org/apache/uniffle/client/ClientUtilsTest.java b/client/src/test/java/org/apache/uniffle/client/ClientUtilsTest.java
index fcadaed7..03139f65 100644
--- a/client/src/test/java/org/apache/uniffle/client/ClientUtilsTest.java
+++ b/client/src/test/java/org/apache/uniffle/client/ClientUtilsTest.java
@@ -22,13 +22,11 @@ import org.junit.jupiter.api.Test;
 import org.apache.uniffle.client.util.ClientUtils;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
 
 public class ClientUtilsTest {
 
-  private static String EXCEPTION_EXPECTED = "Exception excepted";
-
   @Test
   public void getBlockIdTest() {
     // max value of blockId
@@ -40,23 +38,14 @@ public class ClientUtilsTest {
     // min value of blockId
     assertEquals(
         new Long(0L), ClientUtils.getBlockId(0, 0, 0));
-    try {
-      ClientUtils.getBlockId(16777216, 0, 0);
-      fail(EXCEPTION_EXPECTED);
-    } catch (Exception e) {
-      assertTrue(e.getMessage().contains("Can't support partitionId[16777216], the max value should be 16777215"));
-    }
-    try {
-      ClientUtils.getBlockId(0, 2097152, 0);
-      fail(EXCEPTION_EXPECTED);
-    } catch (Exception e) {
-      assertTrue(e.getMessage().contains("Can't support taskAttemptId[2097152], the max value should be 2097151"));
-    }
-    try {
-      ClientUtils.getBlockId(0, 0, 262144);
-      fail(EXCEPTION_EXPECTED);
-    } catch (Exception e) {
-      assertTrue(e.getMessage().contains("Can't support sequence[262144], the max value should be 262143"));
-    }
+
+    final Throwable e1 = assertThrows(IllegalArgumentException.class, () -> ClientUtils.getBlockId(16777216, 0, 0));
+    assertTrue(e1.getMessage().contains("Can't support partitionId[16777216], the max value should be 16777215"));
+
+    final Throwable e2 = assertThrows(IllegalArgumentException.class, () -> ClientUtils.getBlockId(0, 2097152, 0));
+    assertTrue(e2.getMessage().contains("Can't support taskAttemptId[2097152], the max value should be 2097151"));
+
+    final Throwable e3 = assertThrows(IllegalArgumentException.class, () -> ClientUtils.getBlockId(0, 0, 262144));
+    assertTrue(e3.getMessage().contains("Can't support sequence[262144], the max value should be 262143"));
   }
 }