On Tue, Jan 26, 2021 at 1:36 AM Till Rohrmann <trohrm...@apache.org> wrote:
> 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. > $FLINK_PROPERTIES was introduced so that the k8s operators can work with the stock Flink images. Here is an example from FlinkK8sOperator: https://github.com/lyft/flinkk8soperator/blob/c2158e7063c8c34231719926e7ff35ecf030f9f6/examples/wordcount/flink-operator-custom-resource.yaml#L12 The operator would then construct the environment variable with the flink-conf.yaml snippet and the entry point *appends* it to the flink-conf.yaml. This allows for the bulk configuration to override any existing configuration entry in the image. There may be other tooling that generates such CRD, for example from jsonnet. > 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? > ConfigMap would replace the file while we originally wanted to provide the ability to merge the users config over that provided in the image. Cheers, Thomas > > 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 > > > > >> >>> > > > > >> > > > > >> > > > > > > > > > >
