You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uniffle.apache.org by ro...@apache.org on 2022/07/05 02:09:30 UTC
[incubator-uniffle] branch master updated: [Followup] Use asList method in some existing configOptions (#18)
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 8256765 [Followup] Use asList method in some existing configOptions (#18)
8256765 is described below
commit 825676584b20b615fb3c1f563e6f35137a009847
Author: Junfan Zhang <ju...@outlook.com>
AuthorDate: Tue Jul 5 10:09:27 2022 +0800
[Followup] Use asList method in some existing configOptions (#18)
### What changes were proposed in this pull request?
Use asList method in some existing configOptions
### Why are the changes needed?
Directly use the asList method in ConfigOptions to get the config list values, and then avoid splitting values by users.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
UTs.
---
.../apache/uniffle/common/config/ConfigOptions.java | 9 ++++++++-
.../apache/uniffle/common/config/ConfigOptionTest.java | 18 ++++++++++++++++++
.../org/apache/uniffle/coordinator/AccessManager.java | 10 ++++------
.../apache/uniffle/coordinator/CoordinatorConf.java | 5 +++--
.../coordinator/AccessCandidatesCheckerTest.java | 2 +-
.../coordinator/AccessClusterLoadCheckerTest.java | 2 +-
.../apache/uniffle/coordinator/AccessManagerTest.java | 10 +++++-----
.../uniffle/test/AccessCandidatesCheckerHdfsTest.java | 2 +-
.../rss/test/SparkSQLWithDelegationShuffleManager.java | 4 ++--
.../SparkSQLWithDelegationShuffleManagerFallback.java | 6 +++---
.../java/org/apache/uniffle/server/HealthCheck.java | 11 +++++------
.../org/apache/uniffle/server/ShuffleServerConf.java | 3 ++-
.../org/apache/uniffle/server/HealthCheckTest.java | 10 +++++-----
13 files changed, 58 insertions(+), 34 deletions(-)
diff --git a/common/src/main/java/org/apache/uniffle/common/config/ConfigOptions.java b/common/src/main/java/org/apache/uniffle/common/config/ConfigOptions.java
index c6a842b..4a429ac 100644
--- a/common/src/main/java/org/apache/uniffle/common/config/ConfigOptions.java
+++ b/common/src/main/java/org/apache/uniffle/common/config/ConfigOptions.java
@@ -18,11 +18,14 @@
package org.apache.uniffle.common.config;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+
/**
* {@code ConfigOptions} are used to build a {@link ConfigOption}.
* The option is typically built in one of the following pattern:
@@ -237,7 +240,11 @@ public class ConfigOptions {
if (v instanceof List) {
return (List<E>) v;
} else {
- return Arrays.stream(v.toString().split(LIST_SPILTTER))
+ String trimmedVal = v.toString().trim();
+ if (StringUtils.isEmpty(trimmedVal)) {
+ return Collections.emptyList();
+ }
+ return Arrays.stream(trimmedVal.split(LIST_SPILTTER))
.map(s -> atomicConverter.apply(s)).collect(Collectors.toList());
}
};
diff --git a/common/src/test/java/org/apache/uniffle/common/config/ConfigOptionTest.java b/common/src/test/java/org/apache/uniffle/common/config/ConfigOptionTest.java
index dec3fae..a3b75ab 100644
--- a/common/src/test/java/org/apache/uniffle/common/config/ConfigOptionTest.java
+++ b/common/src/test/java/org/apache/uniffle/common/config/ConfigOptionTest.java
@@ -131,6 +131,24 @@ public class ConfigOptionTest {
} catch (IllegalArgumentException illegalArgumentException) {
fail();
}
+
+ // test the empty list
+ final ConfigOption<List<String>> emptyListStringOption = ConfigOptions
+ .key("rss.key5")
+ .stringType()
+ .asList()
+ .noDefaultValue()
+ .withDescription("List config key5");
+
+ List<String> key5Val = conf.get(emptyListStringOption);
+ assertNull(key5Val);
+
+ conf.setString(emptyListStringOption.key(), "");
+ assertEquals(conf.get(emptyListStringOption).size(), 0);
+ conf.setString(emptyListStringOption.key(), ", ");
+ assertEquals(conf.get(emptyListStringOption).size(), 0);
+ conf.setString(emptyListStringOption.key(), " ");
+ assertEquals(conf.get(emptyListStringOption).size(), 0);
}
@Test
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java
index aa4914a..1144ad4 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java
@@ -18,11 +18,10 @@
package org.apache.uniffle.coordinator;
import java.io.IOException;
-import java.util.Arrays;
import java.util.List;
import com.google.common.collect.Lists;
-import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.collections.CollectionUtils;
import org.apache.hadoop.conf.Configuration;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -48,14 +47,13 @@ public class AccessManager {
}
private void init() throws RuntimeException {
- String checkers = coordinatorConf.get(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS);
- if (StringUtils.isEmpty(checkers)) {
+ List<String> checkers = coordinatorConf.get(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS);
+ if (CollectionUtils.isEmpty(checkers)) {
LOG.warn("Access checkers is empty, will not init any checkers.");
return;
}
- String[] names = checkers.trim().split(",");
- accessCheckers = RssUtils.loadExtensions(AccessChecker.class, Arrays.asList(names), this);
+ accessCheckers = RssUtils.loadExtensions(AccessChecker.class, checkers, this);
}
public AccessCheckResult handleAccessRequest(AccessInfo accessInfo) {
diff --git a/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorConf.java b/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorConf.java
index bb897d4..e89b348 100644
--- a/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorConf.java
+++ b/coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorConf.java
@@ -62,10 +62,11 @@ public class CoordinatorConf extends RssBaseConf {
.intType()
.defaultValue(9)
.withDescription("The max number of shuffle server when do the assignment");
- public static final ConfigOption<String> COORDINATOR_ACCESS_CHECKERS = ConfigOptions
+ public static final ConfigOption<List<String>> COORDINATOR_ACCESS_CHECKERS = ConfigOptions
.key("rss.coordinator.access.checkers")
.stringType()
- .defaultValue("org.apache.uniffle.coordinator.AccessClusterLoadChecker")
+ .asList()
+ .defaultValues("org.apache.uniffle.coordinator.AccessClusterLoadChecker")
.withDescription("Access checkers");
public static final ConfigOption<Integer> COORDINATOR_ACCESS_CANDIDATES_UPDATE_INTERVAL_SEC = ConfigOptions
.key("rss.coordinator.access.candidates.updateIntervalSec")
diff --git a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessCandidatesCheckerTest.java b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessCandidatesCheckerTest.java
index 202cb0e..a27385a 100644
--- a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessCandidatesCheckerTest.java
+++ b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessCandidatesCheckerTest.java
@@ -56,7 +56,7 @@ public class AccessCandidatesCheckerTest {
getClass().getClassLoader().getResource("coordinator.conf")).getFile();
CoordinatorConf conf = new CoordinatorConf(filePath);
conf.set(CoordinatorConf.COORDINATOR_ACCESS_CANDIDATES_PATH, tempDir.toURI().toString());
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessCandidatesChecker");
// file load checking at startup
diff --git a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessClusterLoadCheckerTest.java b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessClusterLoadCheckerTest.java
index 5b95930..eb140e2 100644
--- a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessClusterLoadCheckerTest.java
+++ b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessClusterLoadCheckerTest.java
@@ -63,7 +63,7 @@ public class AccessClusterLoadCheckerTest {
final String filePath = Objects.requireNonNull(
getClass().getClassLoader().getResource("coordinator.conf")).getFile();
CoordinatorConf conf = new CoordinatorConf(filePath);
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessClusterLoadChecker");
AccessManager accessManager = new AccessManager(conf, clusterManager, new Configuration());
AccessClusterLoadChecker accessClusterLoadChecker =
diff --git a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessManagerTest.java b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessManagerTest.java
index 551a10d..814aa60 100644
--- a/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessManagerTest.java
+++ b/coordinator/src/test/java/org/apache/uniffle/coordinator/AccessManagerTest.java
@@ -46,14 +46,14 @@ public class AccessManagerTest {
public void test() throws Exception {
// test init
CoordinatorConf conf = new CoordinatorConf();
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS, " , ");
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(), " , ");
try {
new AccessManager(conf, null, new Configuration());
} catch (RuntimeException e) {
String expectedMessage = "Empty classes";
assertTrue(e.getMessage().startsWith(expectedMessage));
}
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"com.Dummy,org.apache.uniffle.coordinator.AccessManagerTest$MockAccessChecker");
try {
new AccessManager(conf, null, new Configuration());
@@ -62,7 +62,7 @@ public class AccessManagerTest {
assertTrue(e.getMessage().startsWith(expectedMessage));
}
// test empty checkers
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS, "");
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(), "");
AccessManager accessManager = new AccessManager(conf, null, new Configuration());
assertTrue(accessManager.handleAccessRequest(
new AccessInfo(String.valueOf(new Random().nextInt()),
@@ -70,13 +70,13 @@ public class AccessManagerTest {
.isSuccess());
accessManager.close();
// test mock checkers
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessManagerTest$MockAccessCheckerAlwaysTrue,");
accessManager = new AccessManager(conf, null, new Configuration());
assertEquals(1, accessManager.getAccessCheckers().size());
assertTrue(accessManager.handleAccessRequest(new AccessInfo("mock1")).isSuccess());
assertTrue(accessManager.handleAccessRequest(new AccessInfo("mock2")).isSuccess());
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessManagerTest$MockAccessCheckerAlwaysTrue,"
+ "org.apache.uniffle.coordinator.AccessManagerTest$MockAccessCheckerAlwaysFalse");
accessManager = new AccessManager(conf, null, new Configuration());
diff --git a/integration-test/common/src/test/java/org/apache/uniffle/test/AccessCandidatesCheckerHdfsTest.java b/integration-test/common/src/test/java/org/apache/uniffle/test/AccessCandidatesCheckerHdfsTest.java
index f821374..436953b 100644
--- a/integration-test/common/src/test/java/org/apache/uniffle/test/AccessCandidatesCheckerHdfsTest.java
+++ b/integration-test/common/src/test/java/org/apache/uniffle/test/AccessCandidatesCheckerHdfsTest.java
@@ -61,7 +61,7 @@ public class AccessCandidatesCheckerHdfsTest extends HdfsTestBase {
CoordinatorConf conf = new CoordinatorConf();
conf.set(CoordinatorConf.COORDINATOR_ACCESS_CANDIDATES_UPDATE_INTERVAL_SEC, 1);
conf.set(CoordinatorConf.COORDINATOR_ACCESS_CANDIDATES_PATH, HDFS_URI);
- conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ conf.setString(CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessCandidatesChecker");
// file load checking at startup
diff --git a/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManager.java b/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManager.java
index ba1236a..c8431f3 100644
--- a/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManager.java
+++ b/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManager.java
@@ -33,7 +33,7 @@ import org.apache.uniffle.coordinator.CoordinatorConf;
import org.apache.uniffle.server.ShuffleServerConf;
import org.apache.uniffle.storage.util.StorageType;
-public class SparkSQLWithDelegationShuffleManager extends SparkSQLTest {
+public class SparkSQLWithDelegationShuffleManager extends org.apache.uniffle.test.SparkSQLTest {
@BeforeAll
public static void setupServers() throws Exception {
@@ -41,7 +41,7 @@ public class SparkSQLWithDelegationShuffleManager extends SparkSQLTest {
SparkSQLWithDelegationShuffleManager.class.getClassLoader().getResource("candidates")).getFile();
CoordinatorConf coordinatorConf = getCoordinatorConf();
coordinatorConf.setString(
- CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessCandidatesChecker,org.apache.uniffle.coordinator.AccessClusterLoadChecker");
coordinatorConf.set(CoordinatorConf.COORDINATOR_ACCESS_CANDIDATES_PATH, candidates);
coordinatorConf.set(CoordinatorConf.COORDINATOR_APP_EXPIRED, 5000L);
diff --git a/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManagerFallback.java b/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManagerFallback.java
index 761c402..1af751d 100644
--- a/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManagerFallback.java
+++ b/integration-test/spark-common/src/test/java/com/tencent/rss/test/SparkSQLWithDelegationShuffleManagerFallback.java
@@ -33,15 +33,15 @@ import org.apache.uniffle.coordinator.CoordinatorConf;
import org.apache.uniffle.server.ShuffleServerConf;
import org.apache.uniffle.storage.util.StorageType;
-public class SparkSQLWithDelegationShuffleManagerFallback extends SparkSQLTest {
+public class SparkSQLWithDelegationShuffleManagerFallback extends org.apache.uniffle.test.SparkSQLTest {
@BeforeAll
public static void setupServers() throws Exception {
final String candidates = Objects.requireNonNull(
- SparkSQLWithDelegationShuffleManager.class.getClassLoader().getResource("candidates")).getFile();
+ org.apache.uniffle.test.SparkSQLWithDelegationShuffleManager.class.getClassLoader().getResource("candidates")).getFile();
CoordinatorConf coordinatorConf = getCoordinatorConf();
coordinatorConf.setString(
- CoordinatorConf.COORDINATOR_ACCESS_CHECKERS,
+ CoordinatorConf.COORDINATOR_ACCESS_CHECKERS.key(),
"org.apache.uniffle.coordinator.AccessCandidatesChecker,org.apache.uniffle.coordinator.AccessClusterLoadChecker");
coordinatorConf.set(CoordinatorConf.COORDINATOR_ACCESS_CANDIDATES_PATH, candidates);
coordinatorConf.set(CoordinatorConf.COORDINATOR_APP_EXPIRED, 5000L);
diff --git a/server/src/main/java/org/apache/uniffle/server/HealthCheck.java b/server/src/main/java/org/apache/uniffle/server/HealthCheck.java
index 7e8b4b3..dd14217 100644
--- a/server/src/main/java/org/apache/uniffle/server/HealthCheck.java
+++ b/server/src/main/java/org/apache/uniffle/server/HealthCheck.java
@@ -25,7 +25,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.Uninterruptibles;
-import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.collections.CollectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -47,15 +47,14 @@ public class HealthCheck {
public HealthCheck(AtomicBoolean isHealthy, ShuffleServerConf conf, List<Checker> buildInCheckers) {
this.isHealthy = isHealthy;
this.checkIntervalMs = conf.getLong(ShuffleServerConf.HEALTH_CHECK_INTERVAL);
- String checkersStr = conf.getString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES);
- if (StringUtils.isEmpty(checkersStr) && buildInCheckers.isEmpty()) {
+ List<String> configuredCheckers = conf.get(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES);
+ if (CollectionUtils.isEmpty(configuredCheckers) && buildInCheckers.isEmpty()) {
throw new IllegalArgumentException("The checkers cannot be empty");
}
checkers.addAll(buildInCheckers);
- if (!StringUtils.isEmpty(checkersStr)) {
- String[] checkerNames = checkersStr.split(",");
+ if (CollectionUtils.isNotEmpty(configuredCheckers)) {
try {
- for (String name : checkerNames) {
+ for (String name : configuredCheckers) {
Class<?> cls = Class.forName(name);
Constructor<?> cons = cls.getConstructor(ShuffleServerConf.class);
checkers.add((Checker) cons.newInstance(conf));
diff --git a/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java b/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
index f3c4e77..ee4b7b0 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
@@ -300,9 +300,10 @@ public class ShuffleServerConf extends RssBaseConf {
.defaultValue(false)
.withDescription("The switch for the health check");
- public static final ConfigOption<String> HEALTH_CHECKER_CLASS_NAMES = ConfigOptions
+ public static final ConfigOption<List<String>> HEALTH_CHECKER_CLASS_NAMES = ConfigOptions
.key("rss.server.health.checker.class.names")
.stringType()
+ .asList()
.noDefaultValue()
.withDescription("The list of the Checker's name");
diff --git a/server/src/test/java/org/apache/uniffle/server/HealthCheckTest.java b/server/src/test/java/org/apache/uniffle/server/HealthCheckTest.java
index 5c81dce..4517c7b 100644
--- a/server/src/test/java/org/apache/uniffle/server/HealthCheckTest.java
+++ b/server/src/test/java/org/apache/uniffle/server/HealthCheckTest.java
@@ -33,9 +33,9 @@ public class HealthCheckTest {
public void buildInCheckerTest() {
ShuffleServerConf conf = new ShuffleServerConf();
assertConf(conf);
- conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES, "");
+ conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES.key(), "");
assertConf(conf);
- conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES, "org.apache.uniffle.server.LocalStorageChecker");
+ conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES.key(), "org.apache.uniffle.server.LocalStorageChecker");
conf.set(ShuffleServerConf.RSS_STORAGE_BASE_PATH, "s1");
conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE, StorageType.HDFS.name());
assertConf(conf);
@@ -62,15 +62,15 @@ public class HealthCheckTest {
public void checkTest() {
AtomicBoolean healthy = new AtomicBoolean(false);
ShuffleServerConf conf = new ShuffleServerConf();
- conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES, HealthyMockChecker.class.getCanonicalName());
+ conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES.key(), HealthyMockChecker.class.getCanonicalName());
HealthCheck checker = new HealthCheck(healthy, conf, Lists.newArrayList());
checker.check();
assertTrue(healthy.get());
- conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES, UnHealthyMockChecker.class.getCanonicalName());
+ conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES.key(), UnHealthyMockChecker.class.getCanonicalName());
checker = new HealthCheck(healthy, conf, Lists.newArrayList());
checker.check();
assertFalse(healthy.get());
- conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES,
+ conf.setString(ShuffleServerConf.HEALTH_CHECKER_CLASS_NAMES.key(),
UnHealthyMockChecker.class.getCanonicalName() + "," + HealthyMockChecker.class.getCanonicalName());
checker = new HealthCheck(healthy, conf, Lists.newArrayList());
checker.check();