Chesnay, thanks for bringing this up. I think it's an alternative that's worthwhile to discuss, although I do think that we should stick to the current proposal in the FLIP.
My main concerns about the FLINK_PROPERTIES approach boil down to the following: * A single env var per config option is a standard language-agnostic approach to configure systems. * Having a single env var that encodes multiple config options makes it cumbersome to change individual entries. * (minor) Explicitly mapping an env var to a config key is explicit (nice!), but also results in some repetition. I don't have much experience actually deploying Flink to production, so I would definitely give higher weight to feedback from people with more experience with production Flink deployments. Cheers, Ufuk On Tue, Jan 26, 2021, at 6:03 AM, Thomas Weise 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 > > > >> >>> > > > >> > > > >> > > > > > >
