> > 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 <suxuanna...@gmail.com>
: 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 <tonysong...@gmail.com> @Xuannan Su <suxuanna...@gmail.com>
, 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
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >
> >
>
