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