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