I'm including Steven and Yang into this discussion (cc'd) who raised some good points in the initial DISCUSS thread. Do you have any opinion on the current discussion here?
On Tue, Jan 26, 2021, at 10:59 AM, Ufuk Celebi wrote: > 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 > > > > >> >>> > > > > >> > > > > >> > > > > > > > > > >
