Sounds make sense. We will do it in the FLIP.
Best,
Rui
On Thu, 21 Dec 2023 at 10:20, Xintong Song <tonysong...@gmail.com> wrote:
> Ideally, public API changes should go through the FLIP process.
>
> I see the point that starting a FLIP for such a tiny change might be
> overkill. However, one could also argue that anything that is too trivial
> to go through a FLIP should also be easy enough to quickly get through the
> process.
>
> In this particular case, since you and Xuannan are working on another FLIP
> regarding the usage of string-keys in configurations, why not make removing
> of the @Deprecated annotation part of that FLIP?
>
> Best,
>
> Xintong
>
>
>
> On Wed, Dec 20, 2023 at 9:27 PM Rui Fan <1996fan...@gmail.com> wrote:
>
> > Thanks Xuannan and Xintong for the quick feedback!
> >
> > > 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.
> >
> > Sure, we can add some comments to guide users to use ConfigOption.
> >
> > If so, I will remove the @Deprecated annotation for
> > `getString(String key, String defaultValue)` method`, and add
> > some comments for it.
> >
> > Also, it's indeed a small change related to Public class(or Api),
> > is voting necessary?
> >
> > Best,
> > Rui
> >
> > On Wed, 20 Dec 2023 at 16:55, Xintong Song <tonysong...@gmail.com>
> wrote:
> >
> > > 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
> > > > >> > >>>>>>>>>>>>>
> > > > >> > >>>>>>>>>>>
> > > > >> > >>>>>>>>>>
> > > > >> > >>>>>>>
> > > > >> > >>>>>>
> > > > >> > >>>>>
> > > > >> > >>>>
> > > > >> > >>>
> > > > >> > >>
> > > > >> > >
> > > > >> >
> > > > >> >
> > > >
> > >
> >
>
