>
> Configuration has a `public <T> T get(ConfigOption<T> option)` method.
> Could we remove all `Xxx getXxx(ConfigOption<Xxx> configOption)` methods?
Note: all `public void setXxx(ConfigOption<Xxx> key, Xxx value)` methods
> can be replaced with `public <T> Configuration set(ConfigOption<T> option,
> T value)` as well.
+1
Best,
Xintong
On Tue, Dec 26, 2023 at 8:44 PM Xintong Song <tonysong...@gmail.com> wrote:
> These features don't have a public option, but they work. I'm not sure
>> whether these features are used by some advanced users.
>> Actually, I think some of them are valuable! For example:
>>
>> - ConfigConstants.YARN_CONTAINER_START_COMMAND_TEMPLATE
>> allows users to define the start command of the yarn container.
>> - FileInputFormat.ENUMERATE_NESTED_FILES_FLAG allows
>> flink job reads all files under the directory even if it has nested
>> directories.
>>
>> This FLIP focuses on the refactor option, I'm afraid these features are
>> used
>> in some production and removing these features will affect some flink
>> jobs.
>> So I prefer to keep these features, WDTY?
>>
>
> First of all, I don't think we should support any knobs that users can
> only learn how to use from reading Flink's internal codes. From this
> perspective, for existing string-keyed knobs that are not mentioned in any
> public documentation, yes we can argue that they are functioning, but we
> can also argue that they are not really exposed to users. That means
> migrating them to ConfigOption is not a pure refactor, but would make
> something that used to be hidden from users now exposed to users. For such
> options, I personally would lean toward not exposing them. If we consider
> them as already exposed, then process-wise there's no problem in
> deprecating some infrequently-used options and removing them in a major
> version bump, and if they are proved needed later we can add them back
> anytime. On the other hand, if we consider them as not yet exposed, then
> removing them later would be a breaking change.
>
>
> Secondly, I don't really come up with any cases where users need to tune
> these knobs. E.g., why would we allow users to customize the yarn container
> start command while we already provide `env.java.opts`? And what would be
> the problem if Flink just doesn't support nested files? And even worse,
> such knobs may provide chances for users to shoot themself in the foot.
> E.g., what if %jvmmem% is missing from a user-provided container start
> command? Admittedly, there might be a small fraction of advanced users that
> know how to use these knobs. However, those users usually have their own
> custom fork of Flink, and it should not be a big problem for them to build
> such abilities by themselves.
>
>
> Taking a step back, if we decide that some of the mentioned knobs are
> really useful, we should at least provide enough descriptions to help users
> understand when and how to use these options. E.g., the current description
> for yarn container command template is far from enough, which does not
> explain what the placeholders mean, what happens if some placeholders are
> missing or if an unknown placeholder is provided.
>
>
> Best,
>
> Xintong
>
>
>
> On Tue, Dec 26, 2023 at 7:39 PM Rui Fan <1996fan...@gmail.com> wrote:
>
>> In addition to what is written in FLIP, I found some methods of
>> Configuration
>> are not necessary. And I wanna hear more thoughts from all of you.
>>
>> Configuration has a `public <T> T get(ConfigOption<T> option)` method.
>> Could we remove all `Xxx getXxx(ConfigOption<Xxx> configOption)` methods?
>> Such as:
>> - public int getInteger(ConfigOption<Integer> configOption)
>> - public String getString(ConfigOption<String> configOption)
>> - public long getLong(ConfigOption<Long> configOption)
>> - etc
>>
>> I prefer users and flink use `get(ConfigOption<T> option)` instead of
>> `getXxx(ConfigOption<Xxx> configOption)` based on some reasons:
>>
>> 1. All callers can replace it directly without any extra effort.
>> `T get(ConfigOption<T> option)` method can replace all
>> `Xxx getXxx(ConfigOption<Xxx> configOption)` methods directly.
>> 2. Callers can call get directly, and users or flink developers
>> don't need to care about should they call getInteger or getString.
>> 3. Flink code is easier to maintain.
>> 4. `T get(ConfigOption<T> option)` is designed later than
>> `Xxx getXxx(ConfigOption<Xxx> configOption)`, I guess if
>> `T get(ConfigOption<T> option)` is designed first,
>> all `Xxx getXxx(ConfigOption<Xxx> configOption)` methods
>> aren't needed.
>>
>> Note: all `public void setXxx(ConfigOption<Xxx> key, Xxx value)` methods
>> can be replaced with `public <T> Configuration set(ConfigOption<T> option,
>> T value)` as well.
>>
>> If they can be marked as deprecated, only `public Xxx
>> getXxx(ConfigOption<Xxx> configOption, Xxx overrideDefault)`
>> is keeped after this FLIP. And the rest of getXxx or setXxx can be removed
>> in 2.0.
>>
>> Looking forward to everyone's feedback and suggestions, thank you!
>>
>> Best,
>> Rui
>>
>> On Tue, Dec 26, 2023 at 7:15 PM Rui Fan <1996fan...@gmail.com> wrote:
>>
>> > Thanks Xintong for the quick feedback and the good suggestions!
>> >
>> > > 1. I think the default value for `TASK_MANAGER_LOG_PATH_KEY` should be
>> > "no
>> > > default". We can explain in the description that if not configured,
>> > > `System.getProperty("log.file")` will be used, but that is not a
>> default
>> > > value.
>> >
>> > Explain it in the description is fine for me.
>> >
>> > > 2. So I wonder if we can simply mark them as deprecated and remove in
>> > 2.0.
>> >
>> > These features don't have a public option, but they work. I'm not sure
>> > whether these features are used by some advanced users.
>> > Actually, I think some of them are valuable! For example:
>> >
>> > - ConfigConstants.YARN_CONTAINER_START_COMMAND_TEMPLATE
>> > allows users to define the start command of the yarn container.
>> > - FileInputFormat.ENUMERATE_NESTED_FILES_FLAG allows
>> > flink job reads all files under the directory even if it has nested
>> > directories.
>> >
>> > This FLIP focuses on the refactor option, I'm afraid these features are
>> > used
>> > in some production and removing these features will affect some flink
>> jobs.
>> > So I prefer to keep these features, WDTY?
>> >
>> > > 3. Simply saying "getting / setting value with string key is
>> discouraged"
>> > > in JavaDoc of get/setString is IMHO a bit confusing. People may have
>> the
>> > > question why would we keep the discouraged interfaces at all. I would
>> > > suggest the following:
>> > > ```
>> > > We encourage users and developers to always use ConfigOption for
>> getting
>> > /
>> > > setting the configurations if possible, for its rich description,
>> type,
>> > > default-value and other supports. The string-key-based getter / setter
>> > > should only be used when ConfigOption is not applicable, e.g., the
>> key is
>> > > programmatically generated in runtime.
>> > > ```
>> >
>> > Suggested comment is good for me, and I'd like to hear the thought from
>> > Xuannan
>> > who wrote the original comment.
>> >
>> > Best,
>> > Rui
>> >
>> > On Tue, Dec 26, 2023 at 5:26 PM Xintong Song <tonysong...@gmail.com>
>> > wrote:
>> >
>> >> Thanks for the efforts, Rui and Xuannan.
>> >>
>> >> I think it's a good idea to migrate string-key configuration accesses
>> to
>> >> ConfigOption-s in general.
>> >>
>> >> I have a few suggestions / questions regarding the FLIP.
>> >>
>> >> 1. I think the default value for `TASK_MANAGER_LOG_PATH_KEY` should be
>> "no
>> >> default". We can explain in the description that if not configured,
>> >> `System.getProperty("log.file")` will be used, but that is not a
>> default
>> >> value.
>> >>
>> >> 2. I wonder if the following string-keys can be simply removed? They
>> are
>> >> neither set by Flink, nor documented anywhere (AFAIK) so that users
>> know
>> >> how to set them. All of them were introduced a long time ago, require
>> >> significant knowledge and familiarity about Flink internals and low
>> level
>> >> details in order to use, and some of them are even private. So I
>> wonder if
>> >> we can simply mark them as deprecated and remove in 2.0.
>> >> - ConfigConstants.YARN_CONTAINER_START_COMMAND_TEMPLATE
>> >> - FileInputFormat.FILE_PARAMETER_KEY
>> >> - FileInputFormat.ENUMERATE_NESTED_FILES_FLAG
>> >> - FileOutputFormat.FILE_PARAMETER_KEY
>> >> - BinaryInputFormat.BLOCK_SIZE_PARAMETER_KEY
>> >> - BinaryOutputFormat.BLOCK_SIZE_PARAMETER_KEY
>> >>
>> >> 3. Simply saying "getting / setting value with string key is
>> discouraged"
>> >> in JavaDoc of get/setString is IMHO a bit confusing. People may have
>> the
>> >> question why would we keep the discouraged interfaces at all. I would
>> >> suggest the following:
>> >> ```
>> >> We encourage users and developers to always use ConfigOption for
>> getting /
>> >> setting the configurations if possible, for its rich description, type,
>> >> default-value and other supports. The string-key-based getter / setter
>> >> should only be used when ConfigOption is not applicable, e.g., the key
>> is
>> >> programmatically generated in runtime.
>> >> ```
>> >>
>> >> WDYT?
>> >>
>> >> Best,
>> >>
>> >> Xintong
>> >>
>> >>
>> >>
>> >> On Tue, Dec 26, 2023 at 4:12 PM Rui Fan <1996fan...@gmail.com> wrote:
>> >>
>> >> > Hi all,
>> >> >
>> >> > Xuannan(cced) and I would like to start a discussion on FLIP-405:
>> >> > FLIP-405: Migrate string configuration key to ConfigOption[1].
>> >> >
>> >> > As Flink progresses to 2.0, we want to enhance the user experience
>> >> > with the existing configuration. In FLIP-77, Flink introduced
>> >> ConfigOption
>> >> > with DataType and strongly encourage users to utilize ConfigOption
>> >> > instead of string keys for accessing and setting Flink
>> configurations.
>> >> > Presently, many string configuration keys have been deprecated and
>> >> > replaced with ConfigOptions; however, some string configuration
>> >> > keys are still in use.
>> >> >
>> >> > To ensure a better experience with the existing configuration in
>> Flink
>> >> > 2.0,
>> >> > this FLIP will migrate all user-facing string configuration keys to
>> >> > ConfigOptions.
>> >> > Additionally, we want to modify the Configuration infrastructure to
>> >> > promote the use of ConfigOption over string configuration keys
>> >> > among developers and users. It's mentioned in a preview thread[2].
>> >> >
>> >> > Looking forward to everyone's feedback and suggestions, thank you!
>> >> >
>> >> > [1] https://cwiki.apache.org/confluence/x/6Yr5E
>> >> > [2] https://lists.apache.org/thread/zzsf7glfcdjcjm1hfo1xdwc6jp37nb3m
>> >> >
>> >> > Best,
>> >> > Rui
>> >> >
>> >>
>> >
>>
>
