You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/07/02 09:01:55 UTC
[GitHub] [incubator-uniffle] zuston opened a new pull request, #9: Introduce the asList method in ConfigOptions
zuston opened a new pull request, #9:
URL: https://github.com/apache/incubator-uniffle/pull/9
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://github.com/Tencent/Firestorm/blob/master/CONTRIBUTING.md
2. Ensure you have added or run the appropriate tests for your PR
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]XXXX Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Introduce the asList method in ConfigOptions
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
It’s necessary when getting the list of values from the config directly.
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
No
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
UT
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912421404
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -581,6 +582,11 @@ public <T> RssConf set(ConfigOption<T> option, T value) {
return this;
}
+ public <T> RssConf setList(ConfigOption<List<T>> option, Object value) {
Review Comment:
You are right. I remove it and reuse the method of `RssConf.set` to set list values.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912466967
##########
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 think there is no need to reuse it. Just split it in ConfigOption is enough.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#issuecomment-1172865236
@jerqi Could u help review?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912434155
##########
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:
And we should allow users to set conf kv in the following usages.
1. conf.set("rss.key", "a,b,c");
2. conf.set(ConfigOption<T> key, T value);
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912459521
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -581,6 +581,11 @@ public <T> RssConf set(ConfigOption<T> option, T value) {
return this;
}
+ public RssConf set(String key, String value) {
Review Comment:
We can reuse method `setString`.
##########
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:
````
public <T> Optional<T> getOptional(ConfigOption<T> option) {
Optional<Object> rawValue = getRawValueFromOption(option);
Class<?> clazz = option.getClazz();
Optional<T> value = rawValue.map(v -> option.convertValue(v, clazz));
return value;
}
```
This is the RssConf's implement. Why do we implement the case like conf.set(listStringOption, List("a", "b", c))?
It's not consistent with others.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912460452
##########
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:
````
public <T> Optional<T> getOptional(ConfigOption<T> option) {
Optional<Object> rawValue = getRawValueFromOption(option);
Class<?> clazz = option.getClazz();
Optional<T> value = rawValue.map(v -> option.convertValue(v, clazz));
return value;
}
```
This is the RssConf's implement. Why don't we implement the case like conf.set(listStringOption, List("a", "b", c))?
It's not consistent with others.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#issuecomment-1172866066
Our RssConf imitate SparkConf's style. SparkConf don't have this function, too. Is it better to split config options on the outside of RssConf?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912461846
##########
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:
This case has been supported in latest commit.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#issuecomment-1172870730
I think it looks RssConf imitate FlinkConf's style.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912433417
##########
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:
Although the RssConf impl looks like the Flink conf, but the convert method is different. That means we shouldn't do the convert action in `getOptional` method, just to init it in `TypedConfigOptionBuilder` or `ListConfigOptionBuilder`.
Now in current PR, i dont implement the case like `conf.set(listStringOption, List("a", "b", c))`. I think i can support it in the `ListConfigOptionBuilder` directly. No need to change the `getOptional` method.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912373979
##########
common/src/main/java/com/tencent/rss/common/config/ConfigOptions.java:
##########
@@ -208,4 +217,69 @@ public ConfigOption<T> noDefaultValue() {
converter);
}
}
+
+ /**
+ * Builder for {@link ConfigOption} of list of type {@link E}.
+ *
+ * @param <E> list element type of the option
+ */
+ public static class ListConfigOptionBuilder<E> {
+ private final String key;
+ private final Class<E> clazz;
+ private final Function<Object, E> atomicConverter;
+ private Function<Object, List<E>> asListConverter;
+
+ public ListConfigOptionBuilder(String key, Class<E> clazz, Function<Object, E> atomicConverter) {
+ this.key = key;
+ this.clazz = clazz;
+ this.atomicConverter = atomicConverter;
+ this.asListConverter = (v) -> {
+ return Arrays.stream(v.toString().split(LIST_SPILTTER))
+ .map(s -> atomicConverter.apply(s)).collect(Collectors.toList());
+ };
+ }
+
+ public ListConfigOptionBuilder checkValue(Function<E, Boolean> checkValueFunc, String errMsg) {
+ Function<Object, List<E>> newConverter = (v) -> {
+ List<E> list = Arrays.stream(v.toString().split(LIST_SPILTTER))
+ .map(s -> atomicConverter.apply(s)).collect(Collectors.toList());
Review Comment:
Flink don’t support checkvalue. But it is added in uniffle TypedConfigOptionBuilder. And i think it’s useful and add it into ListConfigOptionBuilder
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912374032
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -581,6 +582,11 @@ public <T> RssConf set(ConfigOption<T> option, T value) {
return this;
}
+ public <T> RssConf setList(ConfigOption<List<T>> option, Object value) {
Review Comment:
Let me take a look.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#issuecomment-1172999653
Updated. @jerqi
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912465086
##########
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:
If you reuse the Flink's logic, it will also be easy to be reviewed, thanks. If you decide to reuse Flink's implement, you should also reuse the Flink's test case.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912474812
##########
common/src/main/java/com/tencent/rss/common/config/ConfigOptions.java:
##########
@@ -54,6 +57,8 @@
* }</pre>
*/
public class ConfigOptions {
+ private static final String LIST_SPILTTER = ",";
Review Comment:
Do we move to class ListConfigOptionBuilder?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912372476
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -581,6 +582,11 @@ public <T> RssConf set(ConfigOption<T> option, T value) {
return this;
}
+ public <T> RssConf setList(ConfigOption<List<T>> option, Object value) {
Review Comment:
Flink class `Configuration` don't have method `setList`. Could we imitate the Flink api?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912462773
##########
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:
Flink Configuration's implement seems more complete. Method `StructuredOptionsSplitter` support more split characters. Should we reuse Flink's implement?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912424791
##########
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;
}
- public <T> RssConf set(ConfigOption<T> option, T value) {
+ public <T> RssConf set(ConfigOption<T> option, Object value) {
Review Comment:
Why do we use Object? It's better to use class `T`.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912461775
##########
common/src/main/java/com/tencent/rss/common/config/RssConf.java:
##########
@@ -581,6 +581,11 @@ public <T> RssConf set(ConfigOption<T> option, T value) {
return this;
}
+ public RssConf set(String key, String value) {
Review Comment:
Got it.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912371720
##########
common/src/main/java/com/tencent/rss/common/config/ConfigOptions.java:
##########
@@ -208,4 +217,69 @@ public ConfigOption<T> noDefaultValue() {
converter);
}
}
+
+ /**
+ * Builder for {@link ConfigOption} of list of type {@link E}.
+ *
+ * @param <E> list element type of the option
+ */
+ public static class ListConfigOptionBuilder<E> {
+ private final String key;
+ private final Class<E> clazz;
+ private final Function<Object, E> atomicConverter;
+ private Function<Object, List<E>> asListConverter;
+
+ public ListConfigOptionBuilder(String key, Class<E> clazz, Function<Object, E> atomicConverter) {
+ this.key = key;
+ this.clazz = clazz;
+ this.atomicConverter = atomicConverter;
+ this.asListConverter = (v) -> {
+ return Arrays.stream(v.toString().split(LIST_SPILTTER))
+ .map(s -> atomicConverter.apply(s)).collect(Collectors.toList());
+ };
+ }
+
+ public ListConfigOptionBuilder checkValue(Function<E, Boolean> checkValueFunc, String errMsg) {
+ Function<Object, List<E>> newConverter = (v) -> {
+ List<E> list = Arrays.stream(v.toString().split(LIST_SPILTTER))
+ .map(s -> atomicConverter.apply(s)).collect(Collectors.toList());
Review Comment:
Just curious, Do Flink support the method `checkValue`?
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#issuecomment-1172914913
> I think it looks RssConf imitate FlinkConf's style. In Flink, it has asList method.
OK.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912465086
##########
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:
If you reuse the Flink's logic, it will also be easy to be reviewed, thanks.
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #9: Introduce the asList method in ConfigOptions
Posted by GitBox <gi...@apache.org>.
zuston commented on code in PR #9:
URL: https://github.com/apache/incubator-uniffle/pull/9#discussion_r912488649
##########
common/src/main/java/com/tencent/rss/common/config/ConfigOptions.java:
##########
@@ -54,6 +57,8 @@
* }</pre>
*/
public class ConfigOptions {
+ private static final String LIST_SPILTTER = ",";
Review Comment:
Done
--
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: dev-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org