Hi Xintong and Rui, Thanks for the quick feedback and the suggestions.
> 1. I think the default value for `TASK_MANAGER_LOG_PATH_KEY` should be "no > default". I have considered both ways of describing the default value. However, I found out that some of the configurations, such as `web.tmpdir`, put `System.getProperty()` in the default value [1]. Some are putting the description in the default value column[2]. So I just picked the first one. I am fine with either way, so long as they are consistent. WDYT? > 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. > ``` The suggested comment looks good to me. Thanks for the suggestion. I will update the comment in the FLIP. > 2. So I wonder if we can simply mark them as deprecated and remove in 2.0. After some investigation, it turns out those options of input/output format are only publicly exposed in the DataSet docs[2], which is deprecated. Thus, marking them as deprecated and removed in Flink 2.0 looks fine to me. @Rui > Configuration has a `public <T> T get(ConfigOption<T> option)` method. > Could we remove all `Xxx getXxx(ConfigOption<Xxx> configOption)` methods? +1 Only keep the get(ConfigOption<T> option), getOptional(ConfigOption<T> option), and set(ConfigOption<T> option, T value). Best, Xuannan [1] https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/#web-tmpdir [2] https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/#kubernetes-container-image-ref [3] https://nightlies.apache.org/flink/flink-docs-master/docs/dev/dataset/overview/#data-sources On Tue, Dec 26, 2023 at 8:47 PM Xintong Song <tonysong...@gmail.com> wrote: > > > > > 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 > >> >> > > >> >> > >> > > >> > >
