Hi Rui,
I am fine with keeping the `String getString(String key, String
defaultValue)` if more people favor it. However, we should let the
user know that we encourage using ConfigOptions over the string-based
configuration key, as Timo said, we should add the message to `String
getString(String key, String defaultValue)` method.
Best,
Xuannan
On Tue, Dec 19, 2023 at 7:55 PM Rui Fan <1996fan...@gmail.com> wrote:
>
> > > 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?
>
> Hi Xintong, thanks for the good suggestion! Most of the entries can be
> serialized to a json string, and we can only write/read them at once.
> The CACHE_FILE_BLOB_KEY is special, its type is byte[], we need to
> store it by the setBytes/getBytes.
>
> Also, I have an offline discussion with @Xuannan Su : refactoring all code
> that uses String as key requires a separate FLIP. And we will provide
> detailed FLIP later.
>
> --------------------------------------------------------------------------------------
>
> Hi all, thanks everyone for the lively discussion. It's really a trade-off to
> keep "String getString(String key, String defaultValue)" or not.
> (It's not a right or wrong thing.)
>
> Judging from the discussion, most discussants can accept that keeping
> `String getString(String key, String defaultValue)` and depreate the
> rest of `getXxx(String key, Xxx defaultValue)`.
>
> cc @Xintong Song @Xuannan Su , WDYT?
>
> Best,
> Rui
>
> On Fri, Dec 15, 2023 at 11:38 AM Zhu Zhu <reed...@gmail.com> wrote:
>>
>> I think it's not clear whether forcing using ConfigOption would hurt
>> the user experience.
>>
>> Maybe it does at the beginning, because using string keys to access
>> Flink configuration can be simpler for new components/jobs.
>> However, problems may happen later if the configuration usages become
>> more complex, like key renaming, using types other than strings, and
>> other problems that ConfigOption was invented to address.
>>
>> Personally I prefer to encourage the usage of ConfigOption.
>> Jobs should use GlobalJobParameter for custom config, which is different
>> from the Configuration interface. Therefore, Configuration is mostly
>> used in other components/plugins, in which case the long-term maintenance
>> can be important.
>>
>> However, since it is not a right or wrong choice, I'd also be fine
>> to keep the `getString()` method if more devs/users are in favor of it.
>>
>> Thanks,
>> Zhu
>>
>> Timo Walther <twal...@apache.org> 于2023年12月14日周四 17:41写道：
>>
>> > The configuration in Flink is complicated and I fear we won't have
>> > enough capacity to substantially fix it. The introduction of
>> > ReadableConfig, WritableConfig, and typed ConfigOptions was a right step
>> > into making the code more maintainable. From the Flink side, every read
>> > access should go through ConfigOption.
>> >
>> > However, I also understand Gyula pragmatism here because (practically
>> > speaking) users get access `getString()` via `toMap().get()`. So I'm
>> > fine with removing the deprecation for functionality that is available
>> > anyways. We should, however, add the message to `getString()` that this
>> > method is discouraged and `get(ConfigOption)` should be the preferred
>> > way of accessting Configuration.
>> >
>> > In any case we should remove the getInt and related methods.
>> >
>> > Cheers,
>> > Timo
>> >
>> >
>> > On 14.12.23 09:56, Gyula Fóra wrote:
>> > > I see a strong value for user facing configs to use ConfigOption and this
>> > > should definitely be an enforced convention.
>> > >
>> > > However with the Flink project growing and many other components and even
>> > > users using the Configuration object I really don’t think that we should
>> > > “force” this on the users/developers.
>> > >
>> > > If we make fromMap / toMap free with basically no overhead, that is fine
>> > > but otherwise I think it would hurt the user experience to remove the
>> > > simple getters / setters. Temporary configoptions to access strings from
>> > > what is practically string->string map is exactly the kind of unnecessary
>> > > boilerplate that every dev and user wants to avoid.
>> > >
>> > > Gyula
>> > >
>> > > There are many cases where the features of the configoption are really
>> > not
>> > > needed.
>> > >
>> > > On Thu, 14 Dec 2023 at 09:38, Xintong Song <tonysong...@gmail.com>
>> > wrote:
>> > >
>> > >> Hi Gyula,
>> > >>
>> > >> First of all, even if we remove the `getXXX(String key, XXX
>> > defaultValue)`
>> > >> methods, there are still several ways to access the configuration with
>> > >> string-keys.
>> > >>
>> > >> - If one wants to access a specific option, as Rui mentioned,
>> > >> `ConfigOptions.key("xxx").stringType().noDefaultValue()` can be
>> > used.
>> > >> TBH,
>> > >> I can't think of a use case where a temporally created ConfigOption
>> > is
>> > >> preferred over a predefined one. Do you have any examples for that?
>> > >> - If one wants to access the whole configuration set, then `toMap`
>> > or
>> > >> `iterator` might be helpful.
>> > >>
>> > >> It is true that these ways are less convenient than `getXXX(String key,
>> > XXX
>> > >> defaultValue)`, and that's exactly my purpose, to make the key-string
>> > less
>> > >> convenient so that developers choose ConfigOption over it whenever is
>> > >> possible.
>> > >>
>> > >> there will always be cases where a more flexible
>> > >>> dynamic handling is necessary without the added overhead of the toMap
>> > >> logic
>> > >>>
>> > >>
>> > >> I'm not sure about this. I agree there are cases where flexible and
>> > dynamic
>> > >> handling is needed, but maybe "without the added overhead of the toMap
>> > >> logic" is not that necessary?
>> > >>
>> > >> I'd think of this as "encouraging developers to use ConfigOption as
>> > much as
>> > >> possible" vs. "a bit less convenient in 5% of the cases". I guess
>> > there's
>> > >> no right and wrong, just different engineer opinions. While I'm
>> > personally
>> > >> stand with removing the string-key access methods, I'd also be fine with
>> > >> the other way if there are more people in favor of it.
>> > >>
>> > >> Best,
>> > >>
>> > >> Xintong
>> > >>
>> > >>
>> > >>
>> > >> On Thu, Dec 14, 2023 at 3:45 PM Gyula Fóra <gyula.f...@gmail.com>
>> > wrote:
>> > >>
>> > >>> 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
>> > >>>>>>>>>>>>>
>> > >>>>>>>>>>>
>> > >>>>>>>>>>
>> > >>>>>>>
>> > >>>>>>
>> > >>>>>
>> > >>>>
>> > >>>
>> > >>
>> > >
>> >
>> >
