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