[jira] [Comment Edited] (FLINK-16274) Add typed builder methods for setting dynamic configuration on StatefulFunctionsAppContainers

["Igal Shilman (Jira)" <ji...@apache.org>]

    [ 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)