jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912426282
##########
common/src/test/java/com/tencent/rss/common/config/ConfigOptionTest.java:
##########
@@ -22,12 +22,88 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import java.util.List;
+import java.util.function.Function;
+
public class ConfigOptionTest {
+ @Test
+ public void testListTypes() {
+ // test the string type list.
+ final ConfigOption<List<String>> listStringConfigOption = ConfigOptions
+ .key("rss.key1")
+ .stringType()
+ .asList()
+ .defaultValues("h1", "h2")
+ .withDescription("List config key1");
+
+ List<String> defaultValues = listStringConfigOption.defaultValue();
+ assertEquals(2, defaultValues.size());
+ assertSame(String.class, listStringConfigOption.getClazz());
+
+ RssBaseConf conf = new RssBaseConf();
+ conf.set(listStringConfigOption, "a,b,c");
Review Comment:
You can use `conf.set("rss.key1", "a,b,c");`
##########
common/src/test/java/com/tencent/rss/common/config/ConfigOptionTest.java:
##########
@@ -22,12 +22,88 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import java.util.List;
+import java.util.function.Function;
+
public class ConfigOptionTest {
+ @Test
+ public void testListTypes() {
+ // test the string type list.
+ final ConfigOption<List<String>> listStringConfigOption = ConfigOptions
+ .key("rss.key1")
+ .stringType()
+ .asList()
+ .defaultValues("h1", "h2")
+ .withDescription("List config key1");
+
+ List<String> defaultValues = listStringConfigOption.defaultValue();
+ assertEquals(2, defaultValues.size());
+ assertSame(String.class, listStringConfigOption.getClazz());
+
+ RssBaseConf conf = new RssBaseConf();
+ conf.set(listStringConfigOption, "a,b,c");
+
+ List<String> vals = conf.get(listStringConfigOption);
+ assertEquals(3, vals.size());
+ assertEquals(vals.toString(), "[a, b, c]");
+
+ // test the long type list
+ final ConfigOption<List<Long>> listLongConfigOption = ConfigOptions
+ .key("rss.key2")
+ .longType()
+ .asList()
+ .defaultValues(1)
+ .withDescription("List long config key2");
+
+ List<Long> longDefaultVals = listLongConfigOption.defaultValue();
+ assertEquals(longDefaultVals.size(), 1);
+
+ conf.set(listLongConfigOption, "1,2,3");
+ List<Long> longVals = conf.get(listLongConfigOption);
+ assertEquals("[1, 2, 3]", longVals.toString());
Review Comment:
```
assertEquals(Lists.newArrayList(1L, 2L, 3L), LongVals);
```
It seems better.
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -576,7 +576,7 @@ public <T> Optional<T> getOptional(ConfigOption<T> option) {
return value;
}
Review Comment:
I suggest you that you read Flink configuration implement carefully. You
should also modify here.
```
public <T> Optional<T> getOptional(ConfigOption<T> option) {
Optional<Object> rawValue = getRawValueFromOption(option);
Class<?> clazz = option.getClazz();
try {
if (option.isList()) {
return rawValue.map(v -> ConfigurationUtils.convertToList(v,
clazz));
} else {
return rawValue.map(v -> ConfigurationUtils.convertValue(v,
clazz));
}
} catch (Exception e) {
throw new IllegalArgumentException(
String.format(
"Could not parse value '%s' for key '%s'.",
rawValue.map(Object::toString).orElse(""),
option.key()),
e);
}
```
##########
common/src/test/java/com/tencent/rss/common/config/ConfigOptionTest.java:
##########
@@ -22,12 +22,88 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import java.util.List;
+import java.util.function.Function;
+
public class ConfigOptionTest {
+ @Test
+ public void testListTypes() {
+ // test the string type list.
+ final ConfigOption<List<String>> listStringConfigOption = ConfigOptions
+ .key("rss.key1")
+ .stringType()
+ .asList()
+ .defaultValues("h1", "h2")
+ .withDescription("List config key1");
+
+ List<String> defaultValues = listStringConfigOption.defaultValue();
+ assertEquals(2, defaultValues.size());
+ assertSame(String.class, listStringConfigOption.getClazz());
+
+ RssBaseConf conf = new RssBaseConf();
+ conf.set(listStringConfigOption, "a,b,c");
Review Comment:
And we should add a case like :
```
conf.set(listStringOption, List("a", "b", c))
assertEquals("[a , b, c]", conf.get(listStringOption).toString());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
