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

Reply via email to