Sounds good to me.
Best,
Xintong
On Wed, Dec 20, 2023 at 9:40 AM Xuannan Su <suxuanna...@gmail.com> wrote:
> 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
> >> > >>>>>>>>>>>>>
> >> > >>>>>>>>>>>
> >> > >>>>>>>>>>
> >> > >>>>>>>
> >> > >>>>>>
> >> > >>>>>
> >> > >>>>
> >> > >>>
> >> > >>
> >> > >
> >> >
> >> >
>
