It is true that we haven't properly discussed whether to support bulk config changes via a single environment variable or not, Thomas. It is also true that one could move the logic to the Flink processes (more specifically to where the config is loaded).
I am somehow assuming that it is easier for (human) users to specify a single env variable for a config option instead of defining a block of Flink configuration values. I wouldn't know how to properly separate the entries of the configuration values from the top of my head (do you use spaces, commas or newline characters?). But maybe this is a wrong assumption and more of a documentation problem. I can see that for some automated tooling (e.g. K8s operators) it might be easier to not go through the translation process and simply copy what is provided to the tools (I guess `cat flink-conf.yaml`). I am interested why you haven't opted for using ConfigMaps which are mounted as the flink-conf.yaml? Do they have some limitations which did not work in your case? Cheers, Till On Tue, Jan 26, 2021 at 6:04 AM Thomas Weise <t...@apache.org> wrote: > It appears that the discussion in FLIP-161 is primarily focussed on setting > individual configuration entries? > > On the other hand, $FLINK_PROPERTIES (and envsubst) were introduced to set > a fragment or the complete flink-conf.yaml. It's used by at least 2 of the > Flink k8s operators. > > In these cases the configuration is supplied by the user in the flink conf > format. It would be cumbersome to translate that into a set of environment > variables to then translate those back into flink-conf.yaml. > > I remember at the time we discussed that Flink itself could perform the > transformation of the config file. That would eliminate the restriction of > only being applicable to the container entry point script. > > Thomas > > On Mon, Jan 25, 2021 at 11:16 AM Khachatryan Roman < > khachatryan.ro...@gmail.com> wrote: > > > I agree with Till about $FLINK_PROPERTIES being only supported by a Flink > > buildfile. > > Besides that, > > 1. having different ways of configuring different applications doesn't > seem > > convenient to me. For example, Kafka and ZK configured via usual > properties > > and Flink via concatenated one. > > 2. It could also be problematic to re-use the configuration from non-java > > apps (PyFlink?). > > 3. And yes, this can also be used in tests. > > 4. Having this logic in scripts means we have to test scripts instead of > > java/python > > > > I probably missed something but what are your concerns regarding the > "magic > > syntax" (s3.access-key → FLINK_CONFIG_S3_ACCESS__KEY) you mentioned > > earlier? > > > > Regards, > > Roman > > > > > > On Mon, Jan 25, 2021 at 6:48 PM Till Rohrmann <trohrm...@apache.org> > > wrote: > > > > > The problem I see with $FLINK_PROPERTIES is that this is only supported > > by > > > Flink's Docker entrypoint.sh. In fact this variable was introduced as a > > > temporary bridge to allow Docker users to change Flink's configuration > > > dynamically. If we had had a proper way of configuring Flink processes > > via > > > env variables, then we wouldn't have introduced it. > > > > > > Cheers, > > > Till > > > > > > On Mon, Jan 25, 2021 at 5:01 PM Ingo Bürk <i...@ververica.com> wrote: > > > > > > > Hi Chesnay, > > > > > > > > thanks for your input! After some thought I think your proposal > makes a > > > > lot of sense, i.e. if we have one single $FLINK_DYNAMIC_PROPERTIES > > > > environment variable, in a Kubernetes environment we could do > something > > > like > > > > > > > > ``` > > > > env: > > > > - name: S3_KEY > > > > valueFrom: # get from secret > > > > - name: FLINK_DYNAMIC_PROPERTIES > > > > value: -Ds3.access-key=$(S3_KEY) > > > > ``` > > > > > > > > This, much like the substitution approach we rejected previously, > > > requires > > > > "intermediate" variables. However, they're all defined in the same > > scope > > > > here, and this solution certainly has the noteworthy benefit that no > > > "magic > > > > syntax" (s3.access-key → FLINK_CONFIG_S3_ACCESS__KEY) is needed, > which > > I > > > > believe makes for a pretty good user experience here. I think > > personally > > > I > > > > prefer this approach over the approach we currently have in the FLIP, > > but > > > > I'm definitely happy to hear thoughts from the others on this > approach! > > > > Especially maybe also regarding the test randomization point raised > > > > by Khachatryan earlier in the discussion. > > > > > > > > > > > > Regards > > > > Ingo > > > > > > > > On Fri, Jan 22, 2021 at 5:18 PM Chesnay Schepler <ches...@apache.org > > > > > > wrote: > > > > > > > >> The FLIP seems to disregard the existence of dynamic properties, and > > I'm > > > >> wondering why that is the case. > > > >> Don't they fulfill similar roles, in that they allow config options > to > > > >> be set on the command-line? > > > >> > > > >> What use-case do they currently not support? > > > >> I assume it's something along the lines of setting some environment > > > >> variable for containers, but at least for those based on our docker > > > >> images we already have a mechanism to support that. > > > >> > > > >> In any case, wouldn't a single DYNAMIC_PROPERTIES variable suffice > > that > > > >> is automatically evaluated by all scripts? > > > >> Why go through all the hassle with the environment variable names? > > > >> > > > >> On 1/22/2021 3:53 PM, Till Rohrmann wrote: > > > >> > The updated design LGTM as well. Nice work Ingo! > > > >> > > > > >> > Cheers, > > > >> > Till > > > >> > > > > >> > On Fri, Jan 22, 2021 at 3:33 PM Ingo Bürk <i...@ververica.com> > > wrote: > > > >> > > > > >> >> Thanks, Ufuk. I think that makes sense, so I moved it from a > > footnote > > > >> to an > > > >> >> addition to prevent that in the future as well. > > > >> >> > > > >> >> Ingo > > > >> >> > > > >> >> On Fri, Jan 22, 2021 at 3:10 PM Ufuk Celebi <u...@apache.org> > > wrote: > > > >> >> > > > >> >>> LGTM. Let's see what the others think... > > > >> >>> > > > >> >>> On Thu, Jan 21, 2021, at 11:37 AM, Ingo Bürk wrote: > > > >> >>>> Regarding env.java.opts, what special handling is needed there? > > > >> AFAICT > > > >> >>> only > > > >> >>>> the rejected alternative of substituting values would've had an > > > >> effect > > > >> >> on > > > >> >>>> this. > > > >> >>> Makes sense 👍 > > > >> >>> > > > >> >>> From the FLIP: > > > >> >>>> This mapping is not strictly bijective, but cases with > > consecutive > > > >> >>> periods or dashes in the key name are not considered here and > > should > > > >> not > > > >> >>> (reasonably) be allowed. > > > >> >>> > > > >> >>> We could actually enforce such restrictions in the > implementation > > of > > > >> >>> ConfigOption, avoiding any surprises down the line. > > > >> >>> > > > >> >>> – Ufuk > > > >> >>> > > > >> > > > >> > > > > > >
