[FLINK-14494][docs] Add ConfigOption type to the documentation generator #10117

dawidwys · 2019-11-07T15:02:42Z

What is the purpose of the change

It adds generating type column to ConfigOptionsDocGenerator

Verifying this change

Added org.apache.flink.docs.configuration.ConfigOptionsDocGeneratorTest#testCreatingTypes
All previous tests should run

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

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

Documentation

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

dawidwys · 2019-11-07T15:03:13Z

I would appreciate if you had a look @zentol @kl0u

flinkbot · 2019-11-07T15:07:20Z

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 43b2c07 (Wed Dec 04 15:08:06 UTC 2019)

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.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 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

flinkbot · 2019-11-07T15:21:22Z

CI report:

6fb1d5a : FAILURE Build
ab79c31 : SUCCESS Build
0a6385f : SUCCESS Build
0fda59d : SUCCESS Build
43b2c07 : SUCCESS Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

zentol

Something I immediately stumbled on was that quite a lot of types are defined as Strings, where this is more generic that it should be. While we can't fix it for port ranges, paths and classes, for durations and memory sizes it should be possible to provide a better type.

Can we get a screenshot for how type column looks like for an enum?

The completeness check is also failing:

16:50:29.360 [INFO] Running org.apache.flink.docs.configuration.ConfigOptionsDocsCompletenessITCase
16:50:29.659 [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.296 s <<< FAILURE! - in org.apache.flink.docs.configuration.ConfigOptionsDocsCompletenessITCase
16:50:29.659 [ERROR] testFullReferenceCompleteness(org.apache.flink.docs.configuration.ConfigOptionsDocsCompletenessITCase)  Time elapsed: 0.227 s  <<< FAILURE!
java.lang.AssertionError: 
Documentation is outdated, please regenerate it according to the instructions in flink-docs/README.md.
	Problems:
		Documented default of yarn.per-job-cluster.include-user-jar in class org.apache.flink.yarn.configuration.YarnConfigOptions is outdated. Expected: "ORDER" Actual: ORDER
	at org.apache.flink.docs.configuration.ConfigOptionsDocsCompletenessITCase.compareDocumentedAndExistingOptions(ConfigOptionsDocsCompletenessITCase.java:120)
	at org.apache.flink.docs.configuration.ConfigOptionsDocsCompletenessITCase.testFullReferenceCompleteness(ConfigOptionsDocsCompletenessITCase.java:78)

16:50:29.981 [INFO] 
16:50:29.981 [INFO] Results:
16:50:29.981 [INFO] 
16:50:29.981 [ERROR] Failures: 
16:50:29.981 [ERROR]   ConfigOptionsDocsCompletenessITCase.testFullReferenceCompleteness:78->compareDocumentedAndExistingOptions:120 Documentation is outdated, please regenerate it according to the instructions in flink-docs/README.md.
	Problems:
		Documented default of yarn.per-job-cluster.include-user-jar in class org.apache.flink.yarn.configuration.YarnConfigOptions is outdated. Expected: "ORDER" Actual: ORDER

zentol · 2019-11-07T19:48:16Z

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

 		return clazz;
 	}

-	boolean isList() {
+	public boolean isList() {


Unless we believe that these methods provide value to users I'm actually inclined to call these via reflection.

zentol · 2019-11-07T19:49:04Z

flink-docs/src/main/java/org/apache/flink/docs/configuration/ConfigOptionsDocGenerator.java

@@ -69,6 +71,8 @@
 	};

 	static final Set<String> EXCLUSIONS = new HashSet<>(Arrays.asList(
+		"org.apache.flink.configuration.ReadableConfig",


is this actually related to the PR?

Yes and no. This should've been part of the https://issues.apache.org/jira/browse/FLINK-14493
Unfortunately I was not aware of this logic and that the ReadableConfig and WritableConfig will be picked up by the documentation generator. Those are new read-only, write-only interfaces for Configuration. Config tables should not be generated for them.

Then I'd move this change into a separate commit.

zentol · 2019-11-07T19:49:29Z

flink-docs/src/main/java/org/apache/flink/docs/configuration/ConfigOptionsDocGenerator.java

 			"            <td>" + formatter.format(option.description()) + "</td>\n" +
 			"        </tr>\n";
 	}

+	static String typeToHtml(OptionWithMetaInfo optionWithMetaInfo) {


@VisibleForTesting

zentol · 2019-11-07T19:55:01Z

flink-docs/src/main/java/org/apache/flink/docs/configuration/ConfigOptionsDocGenerator.java

+		if (option.getClazz().isEnum()) {
+			return String.format(
+				"<p>%s</p>Possible values: %s",
+				escapeCharacters(type),


the actual class name is overkill imo and mostly an implementation detail; I'd just stick to Enum: Possible values: ...

My only two cents on this one are that if in the future we want users to be able to specify these options also programatically, it would make sense to also have the name of the class.

But I am not so sure about the final result so I am also willing to leave it like this.

I'm with @zentol on that one. I think for programmatic approach its enough that we have that type encoded in the ConfigOption. These docs are rather for defining options from config files.

dawidwys · 2019-11-08T10:30:37Z

Thank you @zentol for your review. Here is a screenshot of an enum option:

Also I am sorry for the failed test. Forgot to regenerate the docs after reverting a temporary change to one of the options

The problem with String in most of the rows is that the value of this column is based on the functionality introduced in https://issues.apache.org/jira/browse/FLINK-14493, where we should provide the type explicitly. So far all the memory, duration options were actually of type string and the parsing happened in place where the option was accessed. I can not see a way to change the type automatically without changing the corresponding ConfigOption and all the places it is accessed. A workaround I can see is to enable this column manually only for groups that have all the ConfigOptions updated. Do you think this would be a better approach?

kl0u · 2019-11-08T15:19:13Z

flink-docs/src/main/java/org/apache/flink/docs/configuration/ConfigOptionsDocGenerator.java

+	static String typeToHtml(OptionWithMetaInfo optionWithMetaInfo) {
+		ConfigOption<?> option = optionWithMetaInfo.option;
+		Class<?> clazz = getClazz(option);
+


Do we expect to have a list of enums or of maps (i.e. any level of "nesting")? If yes, what do we expect to have as the result for the docs here?

That's a good point I forgot about. We do support a single level of nesting. So there might be a List or List. I improved the doc generator to handle those cases as well.

dawidwys · 2019-11-12T08:54:52Z

Could you have another look @zentol when you have some time? What do you think about my answers to the remaining questions.

zentol · 2019-11-13T09:26:13Z

Ok, so must duration/memorysize options are still based on Strings.

In that case I would suggest to still add in the hooks for displaying the type, and sticking with String for the time being, and file follow-up tickets for updating these options to not be based on Strings.

… configuration docs generator

…e information

rmetzger added the review=description? label Nov 7, 2019

kl0u self-assigned this Nov 7, 2019

rmetzger added component=Documentation component=Runtime/Configuration labels Nov 7, 2019

zentol self-assigned this Nov 7, 2019

zentol reviewed Nov 7, 2019

View reviewed changes

kl0u reviewed Nov 8, 2019

View reviewed changes

dawidwys force-pushed the flink-14494 branch from 0a6385f to 0fda59d Compare November 12, 2019 08:53

dawidwys added 3 commits November 13, 2019 15:56

[hotfix][docs, config] Exlcude ReadableConfig and WritableConfig from…

cf4758a

… configuration docs generator

[FLINK-14494][docs] Add ConfigOption type to the documentation generator

3d5dcd3

[FLINK-14494][docs] Regenerated config options documentation with typ…

43b2c07

…e information

dawidwys force-pushed the flink-14494 branch from 0fda59d to 43b2c07 Compare November 13, 2019 15:10

dawidwys merged commit 381892f into apache:master Nov 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14494][docs] Add ConfigOption type to the documentation generator #10117

[FLINK-14494][docs] Add ConfigOption type to the documentation generator #10117

dawidwys commented Nov 7, 2019

dawidwys commented Nov 7, 2019

flinkbot commented Nov 7, 2019 •

edited

flinkbot commented Nov 7, 2019 •

edited

zentol left a comment

zentol Nov 7, 2019

zentol Nov 7, 2019

dawidwys Nov 8, 2019

zentol Nov 13, 2019

zentol Nov 7, 2019

zentol Nov 7, 2019

kl0u Nov 8, 2019

dawidwys Nov 8, 2019

dawidwys commented Nov 8, 2019 •

edited

kl0u Nov 8, 2019

dawidwys Nov 8, 2019

dawidwys commented Nov 12, 2019

zentol commented Nov 13, 2019

[FLINK-14494][docs] Add ConfigOption type to the documentation generator #10117

[FLINK-14494][docs] Add ConfigOption type to the documentation generator #10117

Conversation

dawidwys commented Nov 7, 2019

What is the purpose of the change

Verifying this change

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

Documentation

dawidwys commented Nov 7, 2019

flinkbot commented Nov 7, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Nov 7, 2019 • edited

CI report:

zentol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidwys commented Nov 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidwys commented Nov 12, 2019

zentol commented Nov 13, 2019

flinkbot commented Nov 7, 2019 •

edited

flinkbot commented Nov 7, 2019 •

edited

dawidwys commented Nov 8, 2019 •

edited