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