I might have joined the discussion a bit late.
My two cents:
- I'm good with EVs overwriting the flink-conf.yaml. However, I'm a bit
unsure about EVs overwriting the dynamic properties.
- Currently, dynamic properties are expected to be the final
effective configuration in many places. E.g., the active resource manager
derives task managers' memory sizes from user configuration
(e.g. a min-max
range), and writes the derived actual sizes through dynamic properties
(e.g., set min & max to the same value). Making EVs the highest priority
would require changes from using dynamic properties to using EVs for all
such cases.
- I think dynamic properties might be easier to use than EVs, in
terms of overwriting the configurations. Using dynamic
properties, you can
simply add `-Dkey=value` when executing `flink run`. I'm not
entirely sure
if there's a way to set the EVs in a equally simple way, without
needing to
change the image.
- The naming conversions proposed in the FLIP seems not bijective to me.
There could be problems if the configuration key contains underscores.
- a_b -> FLINK_CONFIG_a_b
- FLINK_CONFIG_a_b -> a.b
Thank you~
Xintong Song
On Tue, Jan 26, 2021 at 6:03 PM Ufuk Celebi <u...@apache.org> wrote:
> 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
> > > > > >> >>>
> > > > > >>
> > > > > >>
> > > > >
> > > >
> > >
> >
>
