[
https://issues.apache.org/jira/browse/FLINK-16274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17045574#comment-17045574
]
Igal Shilman edited comment on FLINK-16274 at 2/26/20 2:22 PM:
---------------------------------------------------------------
Hey [~tzulitai],
as it indeed looks nicer in the code, I have to concerns:
a) Having users (statefun core devs) supplying the flink-conf.yaml as part of
the e2e test makes the e2e test
more likely to catch property renaming in Flink.
b) It requires pulling in a dependency on flink-statefun-core into the test
driving code, that is otherwise independent of the runtime which is a very nice
property for an end-to-end black box test.
Although I don't see immediately a disadvantage to adding this dependency
(since the actual system under test executing within a container with a
different classpath) I wouldn't want to give this property for the reason
described in the issue.
Having said all that, since I haven't wrote an e2e test with the new
abstraction yet, I wasn't feeling the pain,
so if you think the points mentioned above are worth trading for that convince
method then lets do that (especially that the work was already done)
was (Author: igal):
Hey [~tzulitai],
as it indeed looks nicer in the code, I have to concerns:
a) Having users (statefun core devs) supplying the flink-conf.yaml as part of
the e2e test makes the e2e test
more likely to catch property renaming in Flink.
b) It requires pulling in a dependency on flink-statefun-core into the test
driving code, that is otherwise independent of the runtime which is a very nice
property for an end-to-end black box test.
Although I don't see immediately a disadvantage to adding this dependency
(since the actual system under test executing within a container with a
different classpath) I wouldn't want to give this property for the reason
described in the PR.
Having said all that, since I haven't wrote an e2e test with the new
abstraction yet, I wasn't feeling the pain,
so if you think the points mentioned above are worth trading for that convince
method then lets do that (especially that the work was already done)
> Add typed builder methods for setting dynamic configuration on
> StatefulFunctionsAppContainers
> ---------------------------------------------------------------------------------------------
>
> Key: FLINK-16274
> URL: https://issues.apache.org/jira/browse/FLINK-16274
> Project: Flink
> Issue Type: New Feature
> Components: Stateful Functions, Test Infrastructure
> Affects Versions: statefun-1.1
> Reporter: Tzu-Li (Gordon) Tai
> Assignee: Tzu-Li (Gordon) Tai
> Priority: Major
> Labels: pull-request-available
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Excerpt from:
> https://github.com/apache/flink-statefun/pull/32#discussion_r383644382
> Currently, you'd need to provide a complete {{Configuration}} as dynamic
> properties when constructing a {{StatefulFunctionsAppContainers}}.
> It'll be nicer if this is built like this:
> {code}
> public StatefulFunctionsAppContainers verificationApp =
> new StatefulFunctionsAppContainers("sanity-verification", 2)
> .withModuleGlobalConfiguration("kafka-broker",
> kafka.getBootstrapServers())
> .withConfiguration(ConfigOption option, configValue)
> {code}
> And by default the {{StatefulFunctionsAppContainers}} just only has the
> configs in the base template {{flink-conf.yaml}}.
> This would require lazy construction of the containers on {{beforeTest}}.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
