Hi Xintong,
I don’t really see the actual practical benefit from removing the getstring
and setstring low level methods.
I understand that ConfigOptions are nicer for 95% of the cases but from a
technical point of view there will always be cases where a more flexible
dynamic handling is necessary without the added overhead of the toMap logic.
I think it’s the most natural thing for any config abstraction to expose
basic get set methods with a simple key.
What do you think?
Cheers
Gyula
On Thu, 14 Dec 2023 at 08:00, Xintong Song <tonysong...@gmail.com> wrote:
> >
> > 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.
> >
>
> Yes, at least from my side.
>
>
> 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.
> >
>
> True, this one requires a bit more effort to migrate from string-key to
> ConfigOption, but still should be doable. Looking at how the two mentioned
> methods are implemented and used, it seems what is really needed is
> serialization and deserialization of `DistributedCacheEntry`-s. And all the
> entries are always written / read at once. So I think we can serialize the
> whole set of entries into a JSON string (or something similar), and use one
> ConfigOption with a deterministic key for it, rather than having one
> ConfigOption for each field in each entry. WDYT?
>
>
> 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~
> >
>
> Exactly. Actually, Xuannan and a few other colleagues are working on
> reviewing all the existing configuration options. We identified some common
> issues that can potentially be improved, and not using string-key is one of
> them. We are still summarizing the findings, and will bring them to the
> community for discussion once ready. Helping hands is definitely welcomed.
> :)
>
>
> Yeah, `toMap` can solve the problem, and I also mentioned it in the
> > initial mail.
> >
>
> True. Sorry I overlooked it.
>
>
> Best,
>
> Xintong
>
>
>
> On Thu, Dec 14, 2023 at 2:14 PM Rui Fan <1996fan...@gmail.com> wrote:
>
> > 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
> >> > > >>> > >
> >> > > >>>
> >> > > >>
> >> >
> >>
> >
>
