igalshilman commented on a change in pull request #307:
URL: https://github.com/apache/flink-statefun/pull/307#discussion_r824473506
##########
File path:
statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/StatefulFunctionsJob.java
##########
@@ -36,18 +35,8 @@
private static final AtomicInteger FEEDBACK_INVOCATION_ID_SEQ = new
AtomicInteger();
public static void main(String... args) throws Exception {
- ParameterTool parameterTool = ParameterTool.fromArgs(args);
- Map<String, String> globalConfigurations = parameterTool.toMap();
-
- StreamExecutionEnvironment env =
StreamExecutionEnvironment.getExecutionEnvironment();
Review comment:
While I understand what you are trying to do here, I'm not feeling
comfortable with how big of a change that is, because there are some subtitles
with the way configuration and the execution env are assembled and interacting.
For example, the StreamExecutionEnvironment.getExecutionEnvironment() is set
via thread local, and thus shared with a previously (stack wise) configurations
set.
I think however that you have a good point about the command line arguments.
Looking at `StatefulFunctionsConfig` what do you think about the following
plan:
1. Add a getter and a setter for embedded like the other known properties in
that class.
2. When I look at `addAllGlobalConfigurations()` I see that they are simply
added to the map, I think that the same logic needs to be applied for these
configuration exactly as the constructor of this class.
This will allow overriding exactly the same configuration parameter via the
command line.
(The precedence should be: first flink-conf.yaml, and then command line
arguments.
3. Move the validation after obtaining a valid StateFunConfig.
```
StatefulFunctionsConfig stateFunConfig =
StatefulFunctionsConfig.fromFlinkConfiguration(flinkConfig);
stateFunConfig.addAllGlobalConfigurations(globalConfigurations);
stateFunConfig.setProvider(new
StatefulFunctionsUniverses.ClassPathUniverseProvider());
StatefulFunctionsConfigValidator.validate(stateFunConfig, flinkConfig);
// use isEmbedded internally to toggle the relevant validations
```
--
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]
