[FLINK-12491][docs][configuration] Fix incorrect javadoc for path sep… #8418

kezhuw · 2019-05-11T15:07:43Z

…arators of CoreOptions.TMP_DIRS

What is the purpose of the change

Fix incorrect javadoc for path separators part of CoreOptions.TMP_DIRS and ConfigConstants.TASK_MANAGER_TMP_DIR_KEY

Brief change log

Fix incorrect javadoc for path separators part of CoreOptions.TMP_DIRS and ConfigConstants.TASK_MANAGER_TMP_DIR_KEY.
Refactor ConfigurationUtils.splitPaths so we can test both unix and windows path separators without demand on ci test matrix.

Verifying this change

This change added tests and can be verified as follows:

Added test for documented path separators.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

flinkbot · 2019-05-11T15:07:51Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 8a6f4f9 (Fri May 28 06:59:36 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

✅ 1. The [description] looks good.
- Approved by @dawidwys [committer]
❓ 2. There is [consensus] that the contribution should go into to Flink.
❗ 3. Needs [attention] from.
- Needs attention by @dawidwys [committer]
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

dawidwys · 2019-05-13T07:27:33Z

Hi @kezhuw,
Thank you for your contribution. I think your correct that there is a problem in org.apache.flink.configuration.ConfigurationUtils#splitPaths, but rather than adjusting the javadocs (and docs) to the broken code, how about we fix the method?

dawidwys · 2019-05-13T07:34:47Z

@flinkbot approve description

kezhuw · 2019-05-13T14:08:18Z

@dawidwys After dug deeper, I found the history of used path separators:

Initially, the separator was solely ":", it was changed to ˋFile.pathSeparatorˋ in commit 80d6c58#diff-0637c4d13b3c0f4301a62a47bedae39e ( Improved compatibility with Windows )
Then the commit 68100e5#diff-c13f0f23fb00495ef0df245fec2bbbb2 (Allow both comma and double-colon as path separator) change it to ˋ",|"+File.pathSeparatorˋ. The documentation 68100e5#diff-32da23d69ac63fdaae0193ad7de283e3 says that:

(colon ':' on unix) or a comma character ','. An example would be /data1/tmp:/data/tmp:/data3/tmp or /data1/tmp,/data2/tmp,/data3/tmp
After that commit, documentation added in a7c407a#diff-125ce56ea60a577549026859d5d9a4e0 and 76abcaa#diff-c4482d965ffc8a83e8a099e0c7f07a22 say that ˋ|ˋ is supported also which is not true.

I look at ˋflink-conf.yamlˋ in master (518616e https://github.com/apache/flink/blob/master/flink-dist/src/main/resources/flink-conf.yaml#L153), it says that:

Add a delimited list for multiple directories, using the system directory delimiter (colon ':' on unix) or a comma, e.g.: /data1/tmp:/data2/tmp:/data3/tmp

So I think it is a mistake in documentation not code.

Besides the history, I think it is distractive to support more separators.

What do you think ?

dawidwys · 2019-05-14T06:34:11Z

What do you think @aljoscha? Which separators should we support?

kezhuw · 2019-05-14T14:45:04Z

@dawidwys @aljoscha I think current the two path separators serve their "purposes" if there is any:

ˋFile.pathSeparatorˋ is same as os's path separator, it fits target os's convention.
ˋ,ˋ provides os-agnostic path separator, so we can configure separated file paths cross os.

A new os-agnostic path separator introduces nothing but distraction in my opinion.

aljoscha · 2019-05-15T14:06:12Z

Isn't the problem that someone interpreted the | as an actual separator and not as the regex syntax that it is?

kezhuw · 2019-05-15T14:39:56Z

@aljoscha I think this is the case when that javadoc was introduced in commit a7c407a#diff-125ce56ea60a577549026859d5d9a4e0 .

kezhuw · 2019-05-20T14:54:03Z

@dawidwys @aljoscha Hi, is there any consensus on this issue ?

dawidwys

Hi,
I think then we can remove the | from the docs. I will try to review the PR in the coming days. So far I found one place that needs an update.

dawidwys · 2019-05-20T15:13:38Z

flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java

@@ -210,7 +210,7 @@

 	/**
 	 * The config parameter defining the directories for temporary files, separated by
-	 * ",", "|", or the system's {@link java.io.File#pathSeparator}.
+	 * "," or the system's {@link java.io.File#pathSeparator}.


Update also the description in TMP_DIRS.withDescription few lines below.

Have fixed.

kezhuw · 2019-07-09T14:28:38Z

@flinkbot attention @dawidwys

flinkbot · 2019-07-09T22:21:54Z

CI report for commit cafdb91: FAILURE Build

kezhuw · 2019-07-10T14:28:10Z

@dawidwys Could you take time to review this pull request ? It has been more that one month past since your last review comment.

flinkbot · 2019-07-11T02:01:22Z

CI report for commit df6d3f5: SUCCESS Build

flinkbot · 2019-08-06T15:50:01Z

CI report:

df6d3f5 : SUCCESS Build

kezhuw · 2020-12-19T14:01:00Z

@flinkbot run azure

flinkbot · 2020-12-19T14:05:34Z

CI report:

8a6f4f9 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

kezhuw · 2020-12-20T11:32:28Z

@dawidwys @aljoscha Could you have time to review this pr ? We have forgotten it for a long time.

…arators of CoreOptions.TMP_DIRS

kezhuw · 2021-01-02T03:16:51Z

@flinkbot run azure

kezhuw · 2021-01-02T13:13:29Z

@flinkbot run azure

rmetzger added review=description? component=Runtime/Configuration labels May 11, 2019

kezhuw force-pushed the fix-path-separator-documentation branch 2 times, most recently from e1f747d to 15fc612 Compare May 12, 2019 05:59

rmetzger added review=consensus? and removed review=description? labels May 13, 2019

dawidwys reviewed May 20, 2019

View reviewed changes

kezhuw force-pushed the fix-path-separator-documentation branch from cafdb91 to df6d3f5 Compare July 10, 2019 14:22

kezhuw force-pushed the fix-path-separator-documentation branch from df6d3f5 to fe55dec Compare December 19, 2020 13:59

[FLINK-12491][configuration][docs] Fix incorrect javadoc for path sep…

8a6f4f9

…arators of CoreOptions.TMP_DIRS

kezhuw force-pushed the fix-path-separator-documentation branch from fe55dec to 8a6f4f9 Compare January 1, 2021 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12491][docs][configuration] Fix incorrect javadoc for path sep… #8418

[FLINK-12491][docs][configuration] Fix incorrect javadoc for path sep… #8418

kezhuw commented May 11, 2019

flinkbot commented May 11, 2019 •

edited

Loading

dawidwys commented May 13, 2019

dawidwys commented May 13, 2019

kezhuw commented May 13, 2019

dawidwys commented May 14, 2019

kezhuw commented May 14, 2019

aljoscha commented May 15, 2019

kezhuw commented May 15, 2019

kezhuw commented May 20, 2019

dawidwys left a comment

dawidwys May 20, 2019

kezhuw May 21, 2019

kezhuw commented Jul 9, 2019

flinkbot commented Jul 9, 2019

kezhuw commented Jul 10, 2019

flinkbot commented Jul 11, 2019

flinkbot commented Aug 6, 2019 •

edited

Loading

kezhuw commented Dec 19, 2020

flinkbot commented Dec 19, 2020 •

edited

Loading

kezhuw commented Dec 20, 2020

kezhuw commented Jan 2, 2021

kezhuw commented Jan 2, 2021

[FLINK-12491][docs][configuration] Fix incorrect javadoc for path sep… #8418

Are you sure you want to change the base?

[FLINK-12491][docs][configuration] Fix incorrect javadoc for path sep… #8418

Conversation

kezhuw commented May 11, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented May 11, 2019 • edited Loading

Automated Checks

Review Progress

dawidwys commented May 13, 2019

dawidwys commented May 13, 2019

kezhuw commented May 13, 2019

dawidwys commented May 14, 2019

kezhuw commented May 14, 2019

aljoscha commented May 15, 2019

kezhuw commented May 15, 2019

kezhuw commented May 20, 2019

dawidwys left a comment

Choose a reason for hiding this comment

dawidwys May 20, 2019

Choose a reason for hiding this comment

kezhuw May 21, 2019

Choose a reason for hiding this comment

kezhuw commented Jul 9, 2019

flinkbot commented Jul 9, 2019

kezhuw commented Jul 10, 2019

flinkbot commented Jul 11, 2019

flinkbot commented Aug 6, 2019 • edited Loading

CI report:

kezhuw commented Dec 19, 2020

flinkbot commented Dec 19, 2020 • edited Loading

CI report:

kezhuw commented Dec 20, 2020

kezhuw commented Jan 2, 2021

kezhuw commented Jan 2, 2021

flinkbot commented May 11, 2019 •

edited

Loading

flinkbot commented Aug 6, 2019 •

edited

Loading

flinkbot commented Dec 19, 2020 •

edited

Loading