You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/02/24 19:00:57 UTC
[GitHub] [flink-statefun] sjwiesman opened a new pull request #32:
[FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on
StreamPlanEnvironment
sjwiesman opened a new pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32
This is a follow up to #27
Because the statefun-flink-launcher is based on JobClusterEntrypoint it
pulls the StreamGraph using a StreamPlanEnvironment. In this case, the
StreamExecutionEnviornment configuration is set to an empty object.
The only way to pull the actual flink-conf in this case is via
GlobalConfiguration.loadConfiguration(). This is safe because
StreamPlanEnvironment implies job based deployment. In all other cases
the configuration is still pulled via reflection.
I've tested this using statefun images and deploying against a local session cluster.
cc @tzulitai @igalshilman
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai closed pull request #32:
[FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on
StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai closed pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
[suggested as a follow-up, no need for this PR]
It'll be nicer if this is supplied like:
```
public StatefulFunctionsAppContainers verificationApp =
new StatefulFunctionsAppContainers("sanity-verification", 2)
.withModuleGlobalConfiguration("kafka-broker", kafka.getBootstrapServers())
.withConfiguration(ConfigOption option, configValue)
```
And by default the `StatefulFunctionsAppContainers` just only has the configs in the base template `flink-conf.yaml`.
That would eliminate the need for explicitly setting a network alias for the `KafkaContainer` (i.e. less boilerplate code in these e2e tests).
This would require lazy construction of the containers on `beforeTest`, a slightly bigger change that is out-of-scope here. So I suggest not including it in this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383640278
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
It'll be nicer if this is supplied like:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383640278
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
It'll be nicer if this is supplied like:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
[suggested as a follow-up, no need for this PR]
It'll be nicer if this is supplied like:
```
public StatefulFunctionsAppContainers verificationApp =
new StatefulFunctionsAppContainers("sanity-verification", 2)
.withModuleGlobalConfiguration("kafka-broker", kafka.getBootstrapServers())
.withConfiguration(ConfigOption option, configValue)
```
And by default the `StatefulFunctionsAppContainers` just only has the configs in the base template `flink-conf.yaml`.
That would eliminate the need for explicitly setting a network alias for the `KafkaContainer` (i.e. less boilerplate code in these e2e tests).
This would require lazy construction of the containers on `beforeTest`, a slightly bigger change that is out-of-scope here. So I suggest not including it in this PR.
WDYT @sjwiesman ?
I can follow-up on that, but wondering how you feel about this suggestion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
[suggested as a follow-up, no need for this PR]
It'll be nicer if this is supplied like:
```
public StatefulFunctionsAppContainers verificationApp =
new StatefulFunctionsAppContainers("sanity-verification", 2)
.withModuleGlobalConfiguration("kafka-broker", kafka.getBootstrapServers())
.withConfiguration(ConfigOption option, configValue)
```
And by default the `StatefulFunctionsAppContainers` just only has the configs in the base template `flink-conf.yaml`.
That would eliminate the need for explicitly setting a network alias for the `KafkaContainer` (i.e. less boilerplate code in these e2e tests).
This would require lazy construction of the containers on `beforeTest`, a slightly bigger change that is out-of-scope here. So I suggest not including it in this PR.
WDYT @sjwiesman ?
I can offer to follow-up on that, but wondering how you feel about the suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383647373
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/pom.xml
##########
@@ -29,6 +29,7 @@ under the License.
<properties>
<testcontainers.version>1.12.5</testcontainers.version>
+ <flink.version>1.10.0</flink.version>
Review comment:
Ah scratch that 😅 It's only defined in `statefun-flink`.
nevermind then :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
[suggested as a follow-up, no need for this PR]
It'll be nicer if this is supplied like:
```
public StatefulFunctionsAppContainers verificationApp =
new StatefulFunctionsAppContainers("sanity-verification", 2)
.withModuleGlobalConfiguration("kafka-broker", kafka.getBootstrapServers())
.withConfiguration(ConfigOption option, configValue)
```
And by default the `StatefulFunctionsAppContainers` just only has the configs in the base template `flink-conf.yaml`.
That would eliminate the need for explicitly setting a network alias for the `KafkaContainer` (i.e. less boilerplate code in these e2e tests).
This would require lazy construction of the containers on `beforeTest`, a slightly bigger change that is out-of-scope here. So I suggest not including it in this PR.
WDYT @sjwiesman ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/src/test/java/org/apache/flink/statefun/itcases/sanity/SanityVerificationITCase.java
##########
@@ -61,14 +62,20 @@
private static final String CONFLUENT_PLATFORM_VERSION = "5.0.3";
+ private static final Configuration flinkConf = new Configuration();
+
+ static {
+ flinkConf.setString("statefun.module.global-config.kafka-broker", Constants.KAFKA_BROKER_HOST);
+ }
+
@Rule
public KafkaContainer kafka =
new KafkaContainer(CONFLUENT_PLATFORM_VERSION)
.withNetworkAliases(Constants.KAFKA_BROKER_HOST);
@Rule
public StatefulFunctionsAppContainers verificationApp =
- new StatefulFunctionsAppContainers("sanity-verification", 2)
+ new StatefulFunctionsAppContainers("sanity-verification", 2, flinkConf)
Review comment:
[suggested as a follow-up, no need for this PR]
It'll be nicer if this is supplied like:
```
public StatefulFunctionsAppContainers verificationApp =
new StatefulFunctionsAppContainers("sanity-verification", 2)
.withModuleGlobalConfiguration("kafka-broker", kafka.getBootstrapServers())
.withConfiguration(ConfigOption option, configValue)
```
And by default the `StatefulFunctionsAppContainers` just only has the configs in the base template `flink-conf.yaml`.
That would eliminate the need for explicitly setting a network alias for the `KafkaContainer` (i.e. less boilerplate code in these e2e tests).
This would require lazy construction of the containers on `beforeTest`, a slightly bigger change that is out-of-scope here. So I suggest not including it in this PR.
WDYT @sjwiesman ?
I can follow-up on that, but wondering how you feel about the suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#32: [FLINK-16149][hotfix][core] Use
GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #32: [FLINK-16149][hotfix][core] Use GlobalConfiguration.loadConfiguration() on StreamPlanEnvironment
URL: https://github.com/apache/flink-statefun/pull/32#discussion_r383642397
##########
File path: statefun-end-to-end-tests/statefun-sanity-itcase/pom.xml
##########
@@ -29,6 +29,7 @@ under the License.
<properties>
<testcontainers.version>1.12.5</testcontainers.version>
+ <flink.version>1.10.0</flink.version>
Review comment:
I think we don't need to define this here. Parent POM already defines `flink.version` so we should inherit that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services