Thanks Xintong and Xuannan for the feedback!

IIUC, you both prefer using ConfigOption instead of string keys for
all use cases, even internal ones. We can even forcefully delete
these @Depreated methods in Flink-2.0 to guide users or
developers to use ConfigOption.

Using ConfigOption is feasible in all scenarios because ConfigOption
can be easily created via
`ConfigOptions.key("xxx").stringType().noDefaultValue()` even if
there is no predefined ConfigOption.

I noticed that Configuration is used in
DistributedCache#writeFileInfoToConfig and readFileInfoFromConfig
to store some cacheFile meta-information. Their keys are
temporary(key name with number) and it is not convenient
to predefine ConfigOption.

If everyone agrees with this direction, we can start to refactor all
code that uses getXxx(String key, String defaultValue) into
getXxx(ConfigOption<Xxx> configOption), and completely
delete all getXxx(String key, String defaultValue) in 2.0.
And I'm willing to pick it up~

To Xintong:

> I think a `toMap` as suggested by Zhu and Xuannan should solve the
> problem. Alternatively, we may also consider providing an iterator for the
> Configuration.

Yeah, `toMap` can solve the problem, and I also mentioned it in the
initial mail.

Also Providing an iterator is fine, but we don't have a strong
requirement for now. Simple is better, how about considering it
if we have other strong requirements in the future?

Looking forwarding to your feedback, thanks~

Best,
Rui

On Thu, Dec 14, 2023 at 11:36 AM Xintong Song <tonysong...@gmail.com> wrote:

> I'm leaning towards not allowing string-key based configuration access in
> the long term.
>
> I see the Configuration being used in two different ways:
>
> 1. Writing / reading a specific option. In such cases, ConfigOption has
> many advantages compared to string-key, such as explicit value type,
> descriptions, default values, deprecated / fallback keys. I think we should
> encourage, and maybe even enforce, choosing ConfigOption over string-keys
> in such specific option access scenarios. That also applies to internal
> usages, for which the description may not be necessary because we won't
> generate documentation from it but we can still benefit from other
> advantages.
>
> 2. Iterating over all the configured options. In such cases, it is
> currently impractical to find the proper ConfigOption for each configured
> option. I think a `toMap` as suggested by Zhu and Xuannan should solve the
> problem. Alternatively, we may also consider providing an iterator for the
> Configuration.
>
> I think if we want to encourage using ConfigOption in case-1, the most
> effective way is to not allow accessing a specific option with a
> string-key, so that developers not awaring of this discussion won't
> accidentally use it. On the other hand, case-2 is a much rarer use case
> compared to case-1, and given the fact that there are alternative
> approaches, I think we should not compromise case-1 for it.
>
> WDYT?
>
> Best,
>
> Xintong
>
>
>
> On Thu, Dec 14, 2023 at 10:25 AM Xuannan Su <suxuanna...@gmail.com> wrote:
>
> > Hi Rui,
> >
> > We are currently revisiting all the configurations for Flink 2.0, and
> > it turns out that many string-based configurations in
> > `ConfigConstants` are deprecated and have been replaced by
> > `ConfigOptions`. Since `ConfigOptions` offers many advantages over
> > string-based configurations for the end user, I believe we should
> > encourage users to set and get the Flink configuration exclusively
> > with `ConfigOption`. And we are going to eventually replace all the
> > string-based configurations with `ConfigOptions` for this use case.
> >
> > For the first use case you mentioned, I think they are all internal
> usage,
> > and we should aim to replace them with ConfigOptions gradually.
> > Meanwhile, we may consider making those getters/setters for internal
> > use only while the replacement is in progress.
> >
> > For the second use case, IIUC, you need to iterate over all the
> > configurations to replace some old configuration keys with new ones. I
> > believe  `toMap` is suitable for this scenario.
> >
> > Best,
> > Xuannan
> >
> >
> >
> > On Wed, Dec 13, 2023 at 9:04 PM Rui Fan <1996fan...@gmail.com> wrote:
> > >
> > > Thanks Zhu for the quick response!
> > >
> > > > It is not a blocker of the deprecation, epsecially given that they
> are
> > > not standard
> > > > configuration and are just using Configuration class for convenience.
> > >
> > > Yes, it's not a blocker of deprecation.
> > >
> > > > These are internal usages and we can have an internal getter method
> for
> > > them.
> > >
> > > For case1, do you mean we reuse the old getString method as the
> internal
> > > getter method or add a new getter method?
> > >
> > > Anyway, it's fine for me if we have an internal getter method. As I
> > > understand,
> > > the public method without any annotation will be the internal method,
> > right?
> > >
> > > > Not sure why it's required to convert all keys or values. If it is
> used
> > > > to create strings for dynamic properties or config files to deploy
> > jobs,
> > > > toMap()/toFileWritableMap() may be a better choice. They are already
> > > > used in this kind of scenarios.
> > >
> > > For case2, it's really a special scenario, and toMap() is fine for
> case2.
> > > Case2 uses the getString instead of toMap due to getString is easier.
> > > Also, kubernetes-operator is also a internal usage, if case1 is solved,
> > > case2 also can use the internal getter method.So we can focus on case1.
> > >
> > > Thank you
> > >
> > > Best,
> > > Rui
> > >
> > > On Wed, Dec 13, 2023 at 8:01 PM Zhu Zhu <reed...@gmail.com> wrote:
> > >
> > > > Hi Rui,
> > > >
> > > > I'd like to understand why there is a strong requirement for these
> > > > deprecated
> > > > methods. The ConfigOption param methods help to do the type
> conversion
> > so
> > > > that users do not need to do it by themselves.
> > > >
> > > > For the 2 reasons to keep these methods mentioned above:
> > > >
> > > > > 1. A lot of scenarios don't define the ConfigOption, they using
> > > > String as the key and value directly, such as: StreamConfig,
> > > > TaskConfig, DistributedCache, etc.
> > > >
> > > > These are internal usages and we can have an internal getter method
> for
> > > > them.
> > > > It is not a blocker of the deprecation, epsecially given that they
> are
> > not
> > > > standard
> > > > configuration and are just using Configuration class for convenience.
> > > >
> > > > > 2. Some code wanna convert all keys or values, this converting
> > > > is generic, so the getString(String key, String defaultValue) is
> > needed.
> > > > Such as: kubernetes-operator [3].
> > > >
> > > > Not sure why it's required to convert all keys or values. If it is
> used
> > > > to create strings for dynamic properties or config files to deploy
> > jobs,
> > > > toMap()/toFileWritableMap() may be a better choice. They are already
> > > > used in this kind of scenarios.
> > > > If it just needs to read some of the config, why not using the
> proposed
> > > > way to read and parse the config? Pre-defined ConfigOptions are
> better
> > > > for configuration maintenance, compared to arbitrary strings
> > > >
> > > > Thanks,
> > > > Zhu
> > > >
> > > > Rui Fan <1996fan...@gmail.com> 于2023年12月13日周三 19:27写道:
> > > >
> > > >> Thanks Martijn for the quick clarification!
> > > >>
> > > >> I see Zhu Zhu and Junrui Lee are working on configuration related
> > > >> work of Flink-2.0. I would cc them, and hear some thoughts from
> them.
> > > >>
> > > >> Best,
> > > >> Rui
> > > >>
> > > >> On Wed, Dec 13, 2023 at 7:17 PM Martijn Visser <
> > martijnvis...@apache.org>
> > > >> wrote:
> > > >>
> > > >>> Hi Rui,
> > > >>>
> > > >>> I'm more wondering if part of the configuration layer changes would
> > > >>> mean that these APIs would be removed in 2.0. Because if so, then I
> > > >>> don't think we should remove the Deprecate annotation. But I have
> > very
> > > >>> little visibility on the plans for the configuration layer.
> > > >>>
> > > >>> Thanks,
> > > >>>
> > > >>> Martijn
> > > >>>
> > > >>> On Wed, Dec 13, 2023 at 12:15 PM Rui Fan <1996fan...@gmail.com>
> > wrote:
> > > >>> >
> > > >>> > Hi Martijn,
> > > >>> >
> > > >>> > Thanks for your reply!
> > > >>> >
> > > >>> > I noticed the 2.0 is doing some work related to clean
> > configuration.
> > > >>> > But this discussion is different from other work. Most of other
> > work
> > > >>> > are deprecate some Apis in Flink-1.19 and remove them in
> Flink-2.0.
> > > >>> >
> > > >>> > This discussion is a series of methods have been marked to
> > @Deprecate,
> > > >>> > but they are still used so far. I propose remove the @Deprecate
> > > >>> annotation
> > > >>> > and keep these methods.
> > > >>> >
> > > >>> > In brief, I think some deprecated methods shouldn't be
> deprecated.
> > > >>> > WDYT?
> > > >>> >
> > > >>> > Please correct me if I'm wrong, thanks~
> > > >>> >
> > > >>> > Best,
> > > >>> > Rui
> > > >>> >
> > > >>> > On Wed, Dec 13, 2023 at 7:07 PM Martijn Visser <
> > > >>> martijnvis...@apache.org>
> > > >>> > wrote:
> > > >>> >
> > > >>> > > Hi Rui,
> > > >>> > >
> > > >>> > > Are you thinking about this as part of Flink 2.0, since that
> has
> > the
> > > >>> > > goal to introduce a completely clean configuration layer? [1]
> > > >>> > >
> > > >>> > > Best regards,
> > > >>> > >
> > > >>> > > Martijn
> > > >>> > >
> > > >>> > > [1]
> > https://cwiki.apache.org/confluence/display/FLINK/2.0+Release
> > > >>> > >
> > > >>> > > On Wed, Dec 13, 2023 at 11:28 AM Maximilian Michels <
> > m...@apache.org>
> > > >>> > > wrote:
> > > >>> > > >
> > > >>> > > > Hi Rui,
> > > >>> > > >
> > > >>> > > > +1 for removing the @Deprecated annotation from
> > `getString(String
> > > >>> key,
> > > >>> > > > String defaultValue)`. I would remove the other typed
> variants
> > with
> > > >>> > > > default values but I'm ok with keeping them if they are still
> > used.
> > > >>> > > >
> > > >>> > > > -Max
> > > >>> > > >
> > > >>> > > > On Wed, Dec 13, 2023 at 4:59 AM Rui Fan <
> 1996fan...@gmail.com>
> > > >>> wrote:
> > > >>> > > > >
> > > >>> > > > > Hi devs,
> > > >>> > > > >
> > > >>> > > > > I'd like to start a discussion to discuss whether
> > Configuration
> > > >>> > > supports
> > > >>> > > > > getting value based on the String key.
> > > >>> > > > >
> > > >>> > > > > In the FLIP-77[1] and FLINK-14493[2], a series of methods
> of
> > > >>> > > Configuration
> > > >>> > > > > are marked as @Deprecated, for example:
> > > >>> > > > > - public String getString(String key, String defaultValue)
> > > >>> > > > > - public long getLong(String key, long defaultValue)
> > > >>> > > > > - public boolean getBoolean(String key, boolean
> defaultValue)
> > > >>> > > > > - public int getInteger(String key, int defaultValue)
> > > >>> > > > >
> > > >>> > > > > The java doc suggests using getString(ConfigOption, String)
> > or
> > > >>> > > > > getOptional(ConfigOption), it means using ConfigOption as
> key
> > > >>> > > > > instead of String.
> > > >>> > > > >
> > > >>> > > > > They are depreated since Flink-1.10, but these methods
> still
> > > >>> > > > > be used in a lot of code. I think getString(String key,
> > String
> > > >>> > > > > defaultValue)
> > > >>> > > > > shouldn't be deprecated with 2 reasons:
> > > >>> > > > >
> > > >>> > > > > 1. A lot of scenarios don't define the ConfigOption, they
> > using
> > > >>> > > > > String as the key and value directly, such as:
> StreamConfig,
> > > >>> > > > > TaskConfig, DistributedCache, etc.
> > > >>> > > > >
> > > >>> > > > > 2. Some code wanna convert all keys or values, this
> > converting
> > > >>> > > > > is generic, so the getString(String key, String
> > defaultValue) is
> > > >>> > > needed.
> > > >>> > > > > Such as: kubernetes-operator [3].
> > > >>> > > > >
> > > >>> > > > > Based on it, I have 2 solutions:
> > > >>> > > > >
> > > >>> > > > > 1. Removing the @Deprecated for these methods.
> > > >>> > > > >
> > > >>> > > > > 2. Only removing the @Deprecated for `public String
> > > >>> getString(String
> > > >>> > > key,
> > > >>> > > > > String defaultValue)`
> > > >>> > > > > and delete other getXxx(String key, Xxx defaultValue)
> > directly.
> > > >>> > > > > They have been depreated 8 minor versions ago. In general,
> > the
> > > >>> > > > > getString can replace getInteger, getBoolean, etc.
> > > >>> > > > >
> > > >>> > > > > I prefer solution1, because these getXxx methods are used
> for
> > > >>> now,
> > > >>> > > > > they are easy to use and don't bring large maintenance
> costs.
> > > >>> > > > >
> > > >>> > > > > Note: The alternative to public String getString(String
> key,
> > > >>> String
> > > >>> > > > > defaultValue)
> > > >>> > > > > is Configuration.toMap. But the ease of use is not very
> > > >>> convenient.
> > > >>> > > > >
> > > >>> > > > > Looking forward to hear more thoughts about it! Thank you~
> > > >>> > > > > Also, very much looking forward to feedback from Dawid, the
> > > >>> author of
> > > >>> > > > > FLIP-77.
> > > >>> > > > >
> > > >>> > > > > [1] https://cwiki.apache.org/confluence/x/_RPABw
> > > >>> > > > > [2] https://issues.apache.org/jira/browse/FLINK-14493
> > > >>> > > > > [3]
> > > >>> > > > >
> > > >>> > >
> > > >>>
> >
> https://github.com/apache/flink-kubernetes-operator/pull/729/files#r1424811105
> > > >>> > > > >
> > > >>> > > > > Best,
> > > >>> > > > > Rui
> > > >>> > >
> > > >>>
> > > >>
> >
>

Reply via email to