This is an automated email from the ASF dual-hosted git repository.
zuston 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 737bb7f2d [#974] fix(coordinator): Dynamic remote storage conf invalid
for `LegacyClientConfParser` (#1424)
737bb7f2d is described below
commit 737bb7f2df54b499f480710a691e39dba441cd6a
Author: yanmin <[email protected]>
AuthorDate: Mon Jan 8 11:28:36 2024 +0800
[#974] fix(coordinator): Dynamic remote storage conf invalid for
`LegacyClientConfParser` (#1424)
### What changes were proposed in this pull request?
As title
### Why are the changes needed?
Fix: #974
### Does this PR introduce _any_ user-facing change?
Before:
`rss.coordinator.remote.storage.cluster.conf
hadoop-ns01,dfs.hosts=a,b,c,dfs.service=sssss`
will trigger a bug.
After:
`rss.coordinator.remote.storage.cluster.conf
hadoop-ns01,dfs.hosts=a,b,c,dfs.service=sssss`
will work correctly.
Yes.
### How was this patch tested?
unit test
---
.../uniffle/coordinator/util/CoordinatorUtils.java | 33 +++++++++++++++++++++-
.../conf/LegacyClientConfParserTest.java | 6 +++-
.../coordinator/conf/YamlClientConfParserTest.java | 8 ++++--
.../src/test/resources/dynamicClientConf.legacy | 2 +-
.../src/test/resources/dynamicClientConf.yaml | 2 +-
5 files changed, 44 insertions(+), 7 deletions(-)
diff --git
a/coordinator/src/main/java/org/apache/uniffle/coordinator/util/CoordinatorUtils.java
b/coordinator/src/main/java/org/apache/uniffle/coordinator/util/CoordinatorUtils.java
index 34ab82346..453319320 100644
---
a/coordinator/src/main/java/org/apache/uniffle/coordinator/util/CoordinatorUtils.java
+++
b/coordinator/src/main/java/org/apache/uniffle/coordinator/util/CoordinatorUtils.java
@@ -18,8 +18,11 @@
package org.apache.uniffle.coordinator.util;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -141,7 +144,10 @@ public class CoordinatorUtils {
}
for (String s : clusterConfItems) {
- String[] item = s.split(Constants.COMMA_SPLIT_CHAR);
+ Pattern pattern = Pattern.compile("([^,=]+=)");
+ Matcher matcher = pattern.matcher(s);
+ List<Integer> matchIndices = getMatchIndices(matcher);
+ String[] item = splitStringByIndices(s, matchIndices);
if (ArrayUtils.isEmpty(item) || item.length < 2) {
LOG.warn(msg, s);
return Maps.newHashMap();
@@ -167,4 +173,29 @@ public class CoordinatorUtils {
}
return res;
}
+
+ public static List<Integer> getMatchIndices(Matcher matcher) {
+ List<Integer> matchIndices = new ArrayList<>();
+ while (matcher.find()) {
+ matchIndices.add(matcher.start());
+ }
+ return matchIndices.isEmpty() ? Collections.emptyList() : matchIndices;
+ }
+
+ public static String[] splitStringByIndices(String inputString,
List<Integer> indices) {
+ if (indices.get(0) != 0) {
+ indices.add(0, 0);
+ }
+ int length = indices.size();
+ String[] resultArray = new String[length];
+ for (int i = 0; i < length; i++) {
+ int startIndex = indices.get(i);
+ int endIndex = (i < length - 1) ? indices.get(i + 1) :
inputString.length();
+ resultArray[i] =
+ (inputString.charAt(endIndex - 1) == ',')
+ ? inputString.substring(startIndex, endIndex - 1)
+ : inputString.substring(startIndex, endIndex);
+ }
+ return resultArray;
+ }
}
diff --git
a/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/LegacyClientConfParserTest.java
b/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/LegacyClientConfParserTest.java
index 579fc1a97..9a8f30e28 100644
---
a/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/LegacyClientConfParserTest.java
+++
b/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/LegacyClientConfParserTest.java
@@ -32,6 +32,10 @@ public class LegacyClientConfParserTest {
assertEquals("v1", conf.getRssClientConf().get("k1"));
assertEquals("v2", conf.getRssClientConf().get("k2"));
assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://a-ns01").getConfItems().get("k1"));
- assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k1"));
+ assertEquals(
+ "v1,v2,v3",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k1"));
+ assertEquals("v4",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k2"));
+ assertEquals(
+ "v5,v6",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k3"));
}
}
diff --git
a/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/YamlClientConfParserTest.java
b/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/YamlClientConfParserTest.java
index 953b03db3..489465c06 100644
---
a/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/YamlClientConfParserTest.java
+++
b/coordinator/src/test/java/org/apache/uniffle/coordinator/conf/YamlClientConfParserTest.java
@@ -32,7 +32,8 @@ public class YamlClientConfParserTest {
getClass().getClassLoader().getResource("dynamicClientConf.yaml").openStream());
assertEquals("v1", conf.getRssClientConf().get("k1"));
assertEquals("v2", conf.getRssClientConf().get("k2"));
- assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://a-ns01").getConfItems().get("k1"));
+ assertEquals(
+ "v1,v2,v3",
conf.getRemoteStorageInfos().get("hdfs://a-ns01").getConfItems().get("k1"));
assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k1"));
}
@@ -74,7 +75,7 @@ public class YamlClientConfParserTest {
yaml =
"remoteStorageInfos:\n"
+ " hdfs://a-ns01:\n"
- + " k1: v1\n"
+ + " k1: v1,v5\n"
+ " k2: v2\n"
+ " hdfs://x-ns01:\n"
+ " k1: v1\n"
@@ -82,7 +83,8 @@ public class YamlClientConfParserTest {
conf = parser.tryParse(IOUtils.toInputStream(yaml));
assertEquals(0, conf.getRssClientConf().size());
assertEquals(2, conf.getRemoteStorageInfos().size());
- assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://a-ns01").getConfItems().get("k1"));
+ assertEquals(
+ "v1,v5",
conf.getRemoteStorageInfos().get("hdfs://a-ns01").getConfItems().get("k1"));
assertEquals("v1",
conf.getRemoteStorageInfos().get("hdfs://x-ns01").getConfItems().get("k1"));
yaml =
diff --git a/coordinator/src/test/resources/dynamicClientConf.legacy
b/coordinator/src/test/resources/dynamicClientConf.legacy
index 179756ee1..65c970027 100644
--- a/coordinator/src/test/resources/dynamicClientConf.legacy
+++ b/coordinator/src/test/resources/dynamicClientConf.legacy
@@ -19,5 +19,5 @@ k1 v1
k2 v2
rss.coordinator.remote.storage.path hdfs://x-ns01,hdfs://a-ns01
-rss.coordinator.remote.storage.cluster.conf
x-ns01,k1=v1,k2=v2;a-ns01,k1=v1,k2=v2
+rss.coordinator.remote.storage.cluster.conf
x-ns01,k1=v1,v2,v3,k2=v4,k3=v5,v6;a-ns01,k1=v1,k2=v2
diff --git a/coordinator/src/test/resources/dynamicClientConf.yaml
b/coordinator/src/test/resources/dynamicClientConf.yaml
index 16f9829a7..44de0ac7e 100644
--- a/coordinator/src/test/resources/dynamicClientConf.yaml
+++ b/coordinator/src/test/resources/dynamicClientConf.yaml
@@ -24,7 +24,7 @@ remoteStorageInfos:
<configuration>
<property>
<name>k1</name>
- <value>v1</value>
+ <value>v1,v2,v3</value>
</property>
<property>
