[FLINK-15683][docs] Restructure Configuration page #10982

StephanEwen · 2020-01-31T09:45:41Z

What is the purpose of the change

This restructures the documentation page to make it less confusing and more easily digestible for users. This also removes some outdated/confusing/misleading background sections.

The improvements are twofold:

Group the parameters by "use case" and "user-facing behavior", rather than by the class in which they are defined.
Distinguish between parameters that are required or typically use to set up some functionality, and parameters you should not worry about in most cases (only you really need to do advanced trouble-shooting).

Follow-up

This PR does not yet update and descriptions or resolve the issue that (none) as the default value does not tell the user if this is a required parameter (for a certain feature), it has an internal default behavior or fallback, or it is purely optional.

Brief change log

Main changes:

Generalize the @CommonOption annotation to @Section to support putting options in different (multiple) sections independent of the source code file in which they are defined
Rework the configuration.md page to set up a different structure of grouping the options
https://github.com/apache/flink/compare/master...StephanEwen:restructure_docs?expand=1#diff-b98ef45d30f932983679d102102dd8f0

Additional changes:

Removes outdated section on HDFS config variables
Removes outdated background section and places extra links to other parts of the docs in the relevant sections.

flinkbot · 2020-01-31T09:47:23Z

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 f691121 (Fri Jan 31 09:47:23 UTC 2020)

Warnings:

This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

_{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 · 2020-01-31T10:08:38Z

CI report:

2274760 Travis: FAILURE Azure: FAILURE
02696f5 Travis: FAILURE Azure: FAILURE
87b1124 Travis: FAILURE Azure: FAILURE
3129d7f Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

aljoscha

Looks good, I had some remarks about typos, though. This showed me, however, that our naming of cluster components is a bit difficult, because most of the documentation/config options refer to the "Flink Master" as JobManager, which is at odds with the glossary (https://ci.apache.org/projects/flink/flink-docs-master/concepts/glossary.html)

aljoscha · 2020-01-31T10:28:54Z

docs/ops/config.md


-## Common Options
+If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured are automatically discovered.


Suggested change

If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured are automatically discovered.

If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports are automatically discovered.

aljoscha · 2020-01-31T10:29:26Z

docs/ops/config.md


-* This will be replaced by the TOC
-{:toc}
+These options are only necessary for a *standalone* application- or session deployments ([simple standalone]({{site.baseurl}}/ops/deployment/cluster_setup.html) or [Kubernetes]({{site.baseurl}}/ops/deployment/kubernetes.html)).


Suggested change

These options are only necessary for a *standalone* application- or session deployments ([simple standalone]({{site.baseurl}}/ops/deployment/cluster_setup.html) or [Kubernetes]({{site.baseurl}}/ops/deployment/kubernetes.html)).

These options are only necessary for *standalone* application- or session deployments ([simple standalone]({{site.baseurl}}/ops/deployment/cluster_setup.html) or [Kubernetes]({{site.baseurl}}/ops/deployment/kubernetes.html)).

aljoscha · 2020-01-31T10:31:14Z

docs/ops/config.md

+
+    This data is NOT relied upon for persistence/recovery, but if this data gets deleted, it typically causes a heavyweight recovery operation. It is hence recommended to set this to a directory that is not automatically periodically purged.
+
+    Yarn, Mesos, and Kubernets setups automatically configure this value to the local working directories by default.


Suggested change

Yarn, Mesos, and Kubernets setups automatically configure this value to the local working directories by default.

Yarn, Mesos, and Kubernetes setups automatically configure this value to the local working directories by default.

zentol

expert_security_ssl_section.html isn't used anywhere, is this intentional?

zentol · 2020-01-31T10:13:24Z

docs/ops/config.md


-## Common Options
+If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured are automatically discovered.


Suggested change

If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured are automatically discovered.

If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [*active* Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured.

?

zentol · 2020-01-31T10:14:16Z

docs/ops/config.md


-{% include generated/core_configuration.html %}
+The default memory sizes support simple streaming/batch applications, but are too low to yield good performance on more complex applications.


Suggested change

The default memory sizes support simple streaming/batch applications, but are too low to yield good performance on more complex applications.

The default memory sizes support simple streaming/batch applications, but are too low to yield good performance for more complex applications.

zentol · 2020-01-31T12:07:43Z

flink-annotations/src/main/java/org/apache/flink/annotation/docs/Documentation.java


+		String SECTION_COMMON = "common";


common_section.html isn't used anymore, but it seems like some classes are still grouped into a generic common section.

Actually not, no, this variable is unused. Can remove it.

zentol · 2020-01-31T12:12:21Z

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

@@ -33,34 +30,33 @@
 /**
 * The set of configuration options relating to security.
 */
-@PublicEvolving
-@ConfigGroups(groups = {
-	@ConfigGroup(name = "Kerberos", keyPrefix = "security.kerberos"),


Why did you opt for replacing the ConfigGroups here, but kept them for env in CoreOptions?

No specific reason. It is rework step by step. Not everything is migrated in the first step.
At the moment there are still parts that are generated by Class / ConfigGroup, while others are generated by Section option.

In the long run, I would suggest to do everything by section option, that is simpler. But I didn't want just do this without doing a this first rework and see how this model serves us.

section option [are] simpler

This is debatable. The ConfigGroups were written on purpose the way they are so that devs can add new options without having to worry or even know how the docs are generated. You could just add a new kerberos option and it would just work.

Meanwhile, if a dev now adds an option they have to decide whether the option is common/expert, and if they forget to add any of the annotations are at risk of the option not being documented at all.

The core issue I have here is that we introduce an inconsistency in how options are translated into include files, which make it impossible to implement any safeguards to prevent dev errors. In my example above, the generator won't complain about a new ssl option being added without a common/expert annotation, because it will still be added to the file we generate for each config class by default, and the generator assumes that all include files are used in the documentation.

If on the other hand we would completely remove config groups, and the default include file per config class, then we could mandate that every new option must have a section annotation. This we could easily enforce.

zentol · 2020-01-31T12:13:28Z

Please provide a reason for every option that is excluded from the documentation.

zentol · 2020-01-31T12:14:41Z

While the separate into common/expert options makes sense, it seems quite inconvenient to not have them in unified form somewhere. Having to scoll up/down to get a full overview over related options can be rather tiresome :)

StephanEwen · 2020-01-31T15:48:29Z

About the layout / splitting. Would be happy if someone would make another pass after that and further improve this. The stat here is the best that came to my mind and it should already be much better than what we have at the moment, the benefits outweigh the problems by far, so would like to not hold it back for now.

About excluded options: I think I excluded only options that were previously also deprecated / not recommended any more.

web.* options subsumed by rest.*
ssl.* options that should rather be ssl.internal.* and ssl.rest.*
miscellaneous previous oversights.

Do you see some that I accidentally excluded?

StephanEwen · 2020-01-31T15:55:03Z

The "expert_security_ssl_section.htm" was overlooked. Trying to adjust the coverage tests (they currently fail) to make sure these things are called automatically now.

This close apache#15694

…ge to table config options page This closes apache#10957

…igOptionsDocGenerator

…ionOption - Rename '@SectionOption' to '@section' for brevity - Rename '@Section.sections()' to '@Section.value()' That way, the method name can be skipped when only sections (no positions) are specified, which should is the common case after config documentation rework is complete. - Adjust test for @section annotation

StephanEwen · 2020-01-31T17:53:32Z

Pushed a some fixes

including the previously missing generated section expert_security_ssl_section.html
Fixing the tests

zentol · 2020-01-31T19:17:20Z

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

@@ -95,6 +94,7 @@
 	 * @deprecated Use {@link #SSL_INTERNAL_ENABLED} and {@link #SSL_REST_ENABLED} instead.
 	 */
 	@Deprecated
+	@Documentation.ExcludeFromDocumentation


FYI, deprecated options are already excluded from the documentation.

zentol · 2020-01-31T19:34:38Z

About excluded options: I think I excluded only options that were previously [ ... ] not recommended any more.

Looking at the affected options (ssl.*, local.number-resourcemanager) there was, and still is, no indication that they aren't recommended anymore. Anyone looking at the option now just sees a random exclude that may have been added for a myriad of reasons.
Hence my question; all I saw was a perfectly fine looking option being excluded from the docs.

If the ssl.* options aren't supposed to be used anymore they should properly deprecated, likely with deprecated keys on the new options, as should the web option, as should local.number-resourcemanager since (apparently) it isn't even used anymore.

StephanEwen · 2020-01-31T19:38:17Z

I can add a notice why these are excluded.

Side note: the local.number-resourcemanager options (and similar local.*) should never have been a documented option in the first place. These were only used to be passed to the mini cluster, never had any effect in the flink-conf.yaml.

StephanEwen · 2020-01-31T22:42:03Z

Pushed a fix addressing the latest comments.

If the ssl.* options aren't supposed to be used anymore they should properly deprecated, likely with deprecated keys on the new options, as should the web option, as should local.number-resourcemanager since (apparently) it isn't even used anymore.

I think the SSL options are slightly more complex to resolve than just with deprecated keys. That's why I went with only removing them from the config section for now.
The local.number-resourcemanager is indeed not used any more, added a @Deprecated instead.
The web.address option should have been deprecated before (probably overlooked back then). As far as I can tell they are also unused (only referenced as fallback keys).

- Grouping options by semantics and functionality, rather than by defined class. - Splitting between "normal/common options" and options that should only be necessary for trouble-shooting.

…ation docs

StephanEwen · 2020-02-04T07:43:01Z

Merged in

master in d6a7b01
release-1.10 in 5729f23

rmetzger added the review=description? label Jan 31, 2020

StephanEwen force-pushed the restructure_docs branch from f691121 to 2274760 Compare January 31, 2020 09:47

aljoscha self-requested a review January 31, 2020 10:06

aljoscha self-assigned this Jan 31, 2020

StephanEwen force-pushed the restructure_docs branch from 2274760 to 02696f5 Compare January 31, 2020 10:17

rmetzger added component=Documentation component=Runtime/Configuration labels Jan 31, 2020

aljoscha approved these changes Jan 31, 2020

View reviewed changes

zentol self-assigned this Jan 31, 2020

zentol requested changes Jan 31, 2020

View reviewed changes

zentol reviewed Jan 31, 2020

View reviewed changes

StephanEwen force-pushed the restructure_docs branch from 02696f5 to 9664904 Compare January 31, 2020 17:36

bdine and others added 5 commits January 31, 2020 18:38

[FLINK-15694][docs] Remove HDFS Section of Configuration Page

37b7420

This close apache#15694

[FLINK-15697][docs] Move python options from general configuration pa…

b08ba39

…ge to table config options page This closes apache#10957

[FLINK-15824][docs] Generalize CommonOption

199d3f7

[hotfix][docs] Clean up deprecation/unused warnings in tests for Conf…

446dccc

…igOptionsDocGenerator

StephanEwen force-pushed the restructure_docs branch 2 times, most recently from 8e5e3de to 87b1124 Compare January 31, 2020 17:44

zentol reviewed Jan 31, 2020

View reviewed changes

StephanEwen force-pushed the restructure_docs branch from 87b1124 to 9b1b894 Compare January 31, 2020 22:37

StephanEwen added 2 commits January 31, 2020 23:45

[FLINK-15698][docs] Restructure the Configuration docs

ee7697f

- Grouping options by semantics and functionality, rather than by defined class. - Splitting between "normal/common options" and options that should only be necessary for trouble-shooting.

[FLINK-15695][docs] Remove outdated background sections from configur…

3129d7f

…ation docs

StephanEwen force-pushed the restructure_docs branch from 9b1b894 to 3129d7f Compare January 31, 2020 22:46

StephanEwen closed this Feb 4, 2020

StephanEwen deleted the restructure_docs branch February 19, 2020 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15683][docs] Restructure Configuration page #10982

[FLINK-15683][docs] Restructure Configuration page #10982

StephanEwen commented Jan 31, 2020

flinkbot commented Jan 31, 2020

flinkbot commented Jan 31, 2020 •

edited

aljoscha left a comment

aljoscha Jan 31, 2020

aljoscha Jan 31, 2020

aljoscha Jan 31, 2020

zentol left a comment •

edited

zentol Jan 31, 2020

zentol Jan 31, 2020

zentol Jan 31, 2020

StephanEwen Jan 31, 2020

zentol Jan 31, 2020

StephanEwen Jan 31, 2020

zentol Jan 31, 2020 •

edited

zentol Feb 1, 2020

zentol commented Jan 31, 2020

zentol commented Jan 31, 2020 •

edited

StephanEwen commented Jan 31, 2020 •

edited

StephanEwen commented Jan 31, 2020

StephanEwen commented Jan 31, 2020

zentol Jan 31, 2020

zentol commented Jan 31, 2020 •

edited

StephanEwen commented Jan 31, 2020

StephanEwen commented Jan 31, 2020

StephanEwen commented Feb 4, 2020


		## Common Options
		If you use Flink with [Yarn]({{site.baseurl}}/ops/deployment/yarn_setup.html), [Mesos]({{site.baseurl}}/ops/deployment/mesos.html), or the [active Kubernetes integration]({{site.baseurl}}/ops/deployment/native_kubernetes.html), the hostnames and ports get automatically configured are automatically discovered.

	These options are only necessary for a standalone application- or session deployments ([simple standalone]({{site.baseurl}}/ops/deployment/cluster_setup.html) or [Kubernetes]({{site.baseurl}}/ops/deployment/kubernetes.html)).
	These options are only necessary for standalone application- or session deployments ([simple standalone]({{site.baseurl}}/ops/deployment/cluster_setup.html) or [Kubernetes]({{site.baseurl}}/ops/deployment/kubernetes.html)).


		This data is NOT relied upon for persistence/recovery, but if this data gets deleted, it typically causes a heavyweight recovery operation. It is hence recommended to set this to a directory that is not automatically periodically purged.

		Yarn, Mesos, and Kubernets setups automatically configure this value to the local working directories by default.

	Yarn, Mesos, and Kubernets setups automatically configure this value to the local working directories by default.
	Yarn, Mesos, and Kubernetes setups automatically configure this value to the local working directories by default.


		{% include generated/core_configuration.html %}
		The default memory sizes support simple streaming/batch applications, but are too low to yield good performance on more complex applications.

[FLINK-15683][docs] Restructure Configuration page #10982

[FLINK-15683][docs] Restructure Configuration page #10982

Conversation

StephanEwen commented Jan 31, 2020

What is the purpose of the change

Follow-up

Brief change log

flinkbot commented Jan 31, 2020

Automated Checks

Review Progress

flinkbot commented Jan 31, 2020 • edited

CI report:

aljoscha 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

zentol left a comment • edited

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

zentol Jan 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol commented Jan 31, 2020

zentol commented Jan 31, 2020 • edited

StephanEwen commented Jan 31, 2020 • edited

StephanEwen commented Jan 31, 2020

StephanEwen commented Jan 31, 2020

Choose a reason for hiding this comment

zentol commented Jan 31, 2020 • edited

StephanEwen commented Jan 31, 2020

StephanEwen commented Jan 31, 2020

StephanEwen commented Feb 4, 2020

flinkbot commented Jan 31, 2020 •

edited

zentol left a comment •

edited

zentol Jan 31, 2020 •

edited

zentol commented Jan 31, 2020 •

edited

StephanEwen commented Jan 31, 2020 •

edited

zentol commented Jan 31, 2020 •

edited