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
