Hi all, Thank you for all your comments! The FLIP has been updated accordingly. Please let me know if you have any further questions or comments.
Also, note that many people are on Christmas break, so we will keep the discussion open for another week. Best, Xuannan On Wed, Dec 27, 2023 at 5:20 PM Rui Fan <1996fan...@gmail.com> wrote: > > > 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. > > Thanks Xuannan for the detailed investigation, if so, deprecate them > and removing them in Flink 2.0 looks good to me. > > > I think the key of LOCAL_NUMBER_TASK_MANAGER is better as > > 'minicluster.number-of-taskmanagers' or 'minicluster.taskmanager-number' > > instead of 'minicluster.number-taskmanager'. > > Thanks Hang for the good suggestion! 'minicluster.number-of-taskmanagers' > sounds good to me, it's similar to taskmanager.numberOfTaskSlots. > > Best, > Rui > > On Wed, Dec 27, 2023 at 1:56 PM Hang Ruan <ruanhang1...@gmail.com> wrote: >> >> Hi, Rui Fan. >> >> Thanks for this FLIP. >> >> I think the key of LOCAL_NUMBER_TASK_MANAGER is better as >> 'minicluster.number-of-taskmanagers' or 'minicluster.taskmanager-number' >> instead of 'minicluster.number-taskmanager'. >> >> Best, >> Hang >> >> Xuannan Su <suxuanna...@gmail.com> 于2023年12月27日周三 12:40写道： >> >> > 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 >> > > >> >> > >> > > >> >> >> > > >> > >> > > >> >> > > > >> >
