You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/01/02 13:35:13 UTC
[GitHub] [flink] zentol opened a new pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
zentol opened a new pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748
The `ConfigOptionsDocsCompletenessITCase` verifies that all existing and documented options are well-defined; as in that for any key there is exactly 1 default and description present, globally.
There is one use-case however where this check is too strict: reporters. These only work with key suffixes (like "port") and hence are failing this check. As a result we don't use the generator mechanism for reporters, which is unfortunate.
This PR introduces a whitelisting mechanism that allows us to define a set of keys for which this check is not performed.
Note that the remaining checks of the test are still applied. (Is the option documented, does the documented option exist, do the defaults/description match).
The changes are mostly a refactoring of the test. Previously the test was checking whether an ambiguous key exists during the discovery of keys, which was now separated.
This implies that during discovery we no longer end up with a 1:1 mapping of key:option, but rather 1:N which is later checked.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363054887
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
> If some developers happen to create the same config key in different classes, I‘m afraid there exists some risk that user configure that key in flink-conf.yaml but taken effect by another unexpected class.
That's exactly what the current implementation is preventing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r365536939
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,105 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ "host",
+ "port"
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
+ // we allow multiple instances of ConfigOptions with the same key if they are identical
+ return option1;
+ } else {
+ throw duplicateHandler.apply(option1, option2);
+ }
+ }));
+ }
+
+ private static void compareDocumentedAndExistingOptions(Map<String, List<DocumentedOption>> documentedOptions, Map<String, List<ExistingOption>> existingOptions) {
final Collection<String> problems = new ArrayList<>(0);
// first check that all existing options are properly documented
- existingOptions.forEach((key, supposedState) -> {
- DocumentedOption documentedState = documentedOptions.remove(key);
-
- // if nothing matches the docs for this option are up-to-date
- if (documentedState == null) {
- // option is not documented at all
- problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
- } else if (!supposedState.defaultValue.equals(documentedState.defaultValue)) {
- // default is outdated
- problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated. Expected: " + supposedState.defaultValue + " Actual: " + documentedState.defaultValue);
- } else if (!supposedState.description.equals(documentedState.description)) {
- // description is outdated
- problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated.");
+ existingOptions.forEach((key, supposedStates) -> {
+ List<DocumentedOption> documentedState = documentedOptions.remove(key);
+
+ for (ExistingOption supposedState : supposedStates) {
+ if (documentedState == null || documentedState.isEmpty()) {
+ // option is not documented at all
+ problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.defaultValue.equals(documentedOption.defaultValue))) {
+ // default is outdated
+ problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated. Expected: " + supposedState.defaultValue);
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.description.equals(documentedOption.description))) {
+ // description is outdated
+ problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated.");
+ } else {
+ // the docs for this option are up-to-date
+ }
Review comment:
That is checked just below this block by verifying that the set of documented options (effectively the set of _unmatched_ options) is empty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363283498
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
Thanks for your interpretation, I still reserve my judgment on this problem and I agree this should be out of scope of this PR since this problem has been existed for a while.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r366484125
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,105 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ "host",
+ "port"
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
+ // we allow multiple instances of ConfigOptions with the same key if they are identical
+ return option1;
+ } else {
+ throw duplicateHandler.apply(option1, option2);
+ }
+ }));
+ }
+
+ private static void compareDocumentedAndExistingOptions(Map<String, List<DocumentedOption>> documentedOptions, Map<String, List<ExistingOption>> existingOptions) {
final Collection<String> problems = new ArrayList<>(0);
// first check that all existing options are properly documented
- existingOptions.forEach((key, supposedState) -> {
- DocumentedOption documentedState = documentedOptions.remove(key);
-
- // if nothing matches the docs for this option are up-to-date
- if (documentedState == null) {
- // option is not documented at all
- problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
- } else if (!supposedState.defaultValue.equals(documentedState.defaultValue)) {
- // default is outdated
- problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated. Expected: " + supposedState.defaultValue + " Actual: " + documentedState.defaultValue);
- } else if (!supposedState.description.equals(documentedState.description)) {
- // description is outdated
- problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated.");
+ existingOptions.forEach((key, supposedStates) -> {
+ List<DocumentedOption> documentedState = documentedOptions.remove(key);
+
+ for (ExistingOption supposedState : supposedStates) {
+ if (documentedState == null || documentedState.isEmpty()) {
+ // option is not documented at all
+ problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.defaultValue.equals(documentedOption.defaultValue))) {
+ // default is outdated
+ problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated. Expected: " + supposedState.defaultValue);
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.description.equals(documentedOption.description))) {
+ // description is outdated
+ problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated.");
+ } else {
+ // the docs for this option are up-to-date
+ }
Review comment:
Yes, this is what I meant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
* 3e0617bd355a1e8081ebb9ce615365cb8296dbf2 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363101760
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
ah, so _that's_ the case you were asking about.
That's legal. IIRC, when we introduced the generator we actually had an option that was mirroring another one. One could also argue that logically, if code-structure must not have semantics, duplication of code must be allowed.
However, it seems unlikely that 2 options are being created for which they
1. share a key
2. have an _exactly_ matching description (supposedly written in isolation from each other)
3. are semantically so aligned that the key/description are still meaningful
4. yet are semantically so different that the accidental configuration can cause problems.
Overall I think this is mostly a theoretical issue due to us usually providing a component context via the key; to take your example, all `RocksDBOptions` have `rocksdb` in their key, with the inverse rightfully being the case for the `CheckpointingOptions`.
Note that in any case the behavior in this regard isn't different between `master` and this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363101760
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
That's legal. IIRC, when we introduced the generator we actually had an option that was mirroring another one. One could also argue that logically, if code-structure must not have semantics, duplication of code must be allowed.
However, it seems unlikely that 2 options are being created for which they
1. share a key
2. have an _exactly_ matching description (supposedly written in isolation from each other)
3. are semantically so aligned that the key/description are still meaningful
4. yet are semantically so different that the accidental configuration can cause problems.
Overall I think this is mostly a theoretical issue due to us usually providing a component context via the key; to take your example, all `RocksDBOptions` have `rocksdb` in their key, with the inverse rightfully being the case for the `CheckpointingOptions`.
Note that in any case the behavior in this regard isn't different between `master` and this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363042689
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
If some developers happen to create the same config key in different classes, I‘m afraid there exists some risk that user configure that key in `flink-conf.yaml` but taken effect by another unexpected class.
By the way, after introducing data type for config option, we did not include that field to compare in [duplication check](https://github.com/apache/flink/blob/df5eb214294f09a3cd7fcc04ab2617a321041e9d/flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java#L219), and this is certainly out of scope of this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363054887
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
> If some developers happen to create the same config key in different classes, I‘m afraid there exists some risk that user configure that key in flink-conf.yaml but taken effect by another unexpected class.
That's exactly what the current implementationk is preventing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r362571218
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
Review comment:
I think we missed `host` and `port` here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-574298374
Just another thought: Maybe one could add the prefix for suffix options as a non visible attribute to the key field of a configuration option row in the generated html output. Or we indeed say that we omit check `1)`.
In the latter case one would need to find the one matching option out of a collection of `DocumentedOption` with an `ExistingOption`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145014752 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
* 3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145014752) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570209724
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 38514c02c79ab0ddb7e80bb483b6034a40d4e199 (Thu Jan 02 13:39:04 UTC 2020)
**Warnings:**
* **1 pom.xml files were touched**: Check for build and licensing issues.
* Documentation files were touched, but no `.zh.md` files: Update Chinese documentation or file Jira ticket.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## 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](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
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 <summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r368000092
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -139,22 +139,42 @@ private static void compareDocumentedAndExistingOptions(Map<String, List<Documen
// first check that all existing options are properly documented
existingOptions.forEach((key, supposedStates) -> {
- List<DocumentedOption> documentedState = documentedOptions.remove(key);
+ List<DocumentedOption> documentedState = documentedOptions.get(key);
for (ExistingOption supposedState : supposedStates) {
if (documentedState == null || documentedState.isEmpty()) {
// option is not documented at all
problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
- } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.defaultValue.equals(documentedOption.defaultValue))) {
- // default is outdated
- problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated. Expected: " + supposedState.defaultValue);
- } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.description.equals(documentedOption.description))) {
- // description is outdated
- problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated.");
} else {
- // the docs for this option are up-to-date
+ final Iterator<DocumentedOption> candidates = documentedState.iterator();
+
+ boolean matchingDefaultFound = false;
+ boolean matchingDescriptionFound = false;
+ while (candidates.hasNext()) {
+ DocumentedOption candidate = candidates.next();
+ if (supposedState.defaultValue.equals(candidate.defaultValue)) {
+ matchingDefaultFound = true;
+ }
+ if (supposedState.description.equals(candidate.description)) {
+ matchingDescriptionFound = true;
+ }
+ if (matchingDefaultFound && matchingDescriptionFound) {
+ // option is properly documented
+ candidates.remove();
+ break;
+ }
+ }
Review comment:
I'm not sure whether I completely understand this logic here. Can't it happen that we have two options where one fulfills the default value and the other fulfills the description? In this case we would remove the second option and not report any problems even though both of them should cause a problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/145014752 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
* 3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/145014752) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r362573650
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
I think we should better include `containingClass` to compare when calling `ExistingOption#equals` so that these two `ExistingOption`s are really identical.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 UNKNOWN
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol merged pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol merged pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] tillrohrmann commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
tillrohrmann commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r365309969
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,105 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ "host",
+ "port"
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
+ // we allow multiple instances of ConfigOptions with the same key if they are identical
+ return option1;
+ } else {
+ throw duplicateHandler.apply(option1, option2);
+ }
+ }));
+ }
+
+ private static void compareDocumentedAndExistingOptions(Map<String, List<DocumentedOption>> documentedOptions, Map<String, List<ExistingOption>> existingOptions) {
final Collection<String> problems = new ArrayList<>(0);
// first check that all existing options are properly documented
- existingOptions.forEach((key, supposedState) -> {
- DocumentedOption documentedState = documentedOptions.remove(key);
-
- // if nothing matches the docs for this option are up-to-date
- if (documentedState == null) {
- // option is not documented at all
- problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
- } else if (!supposedState.defaultValue.equals(documentedState.defaultValue)) {
- // default is outdated
- problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated. Expected: " + supposedState.defaultValue + " Actual: " + documentedState.defaultValue);
- } else if (!supposedState.description.equals(documentedState.description)) {
- // description is outdated
- problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated.");
+ existingOptions.forEach((key, supposedStates) -> {
+ List<DocumentedOption> documentedState = documentedOptions.remove(key);
+
+ for (ExistingOption supposedState : supposedStates) {
+ if (documentedState == null || documentedState.isEmpty()) {
+ // option is not documented at all
+ problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.defaultValue.equals(documentedOption.defaultValue))) {
+ // default is outdated
+ problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated. Expected: " + supposedState.defaultValue);
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.description.equals(documentedOption.description))) {
+ // description is outdated
+ problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated.");
+ } else {
+ // the docs for this option are up-to-date
+ }
Review comment:
I think we won't fail if we have a `DocumentedOption` which has no corresponding `ExistingOption` since we are only looking whether there is a `DocumentedOption` for every `ExistingOption`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Myasuka commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
Myasuka commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363090356
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
Would you please kindly explain whether the scenario below is legal in Flink?
We define two config option named as `a.b.c` with the same default value and description in different options files (e.g in `CheckpointingOptions` and `RocksDBOptions`). Current `ConfigOptionsDocsCompletenessITCase` would still pass successfully.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on issue #10748: [FLINK-15458][conf][docs]
Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-573345391
@tillrohrmann
An annotation-based approach was my first instinct as well but that doesn't work with the existing checks the test makes.
At it's core, the test does 3 things, which with the structure is obvious:
```
1) assertDocumentedOptionsAreWellDefined(documentedOptions);
2) assertExistingOptionsAreWellDefined(existingOptions);
3) compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
```
The check for documented options being well-defined is where I ran into issues; since annotations are only available for the actual ConfigOption there was no way to differentiate between normal and suffix options when you only look at the documentation.
Including the prefix in the documentation can solve it, but I don't want them in the documentation as they add a lot of noise and the reporter-specific configuration page becomes a lot less concise.
However, when you look at the core test structure, and think about it (as I just did trying to recall why I didn't go with annotations), then check 1) should be redundant. If all existing options are well-defined, and the documented and existing options match, then we can conclude that the documented options are also well-defined.
As such, if we were to remove this check then we could in fact go with an annotation approach.
I'll think about if a bit more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r362788785
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
That would violate our definition of begin well-defined; from the javadocs:
`no 2 options exist for the same key with different descriptions/default values`
The organization into separate classes that we have is only for developer convenience and readability only; it does not (and must not) have any semantics attached to it. Hence the containing class doesn't matter.
The reason for this is simplicity; we don't have to worry about
* options clashing in their default value, which is difficult to document in a good way and can lead to subtle issues when de-duplicating options
* options clashing in their description, which is also difficult to document and usually leads to stale documentation at one place
* options clashing in their type, which may result in a component being unusable when used in conjunction with another
* users not being able to configure distinct values per option
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363277536
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
It is also not really the responsibility of this test to forbid this; which is apparent when you consider that whether you de-duplicate the option or not, the contents of the documentation remain the same.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/144983581 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:f6a0b8e7a74209ec6a22942a28c763207c27d503 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451 TriggerType:PUSH TriggerID:f6a0b8e7a74209ec6a22942a28c763207c27d503
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/145014752 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
Hash:3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452 TriggerType:PUSH TriggerID:3e0617bd355a1e8081ebb9ce615365cb8296dbf2
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
* f6a0b8e7a74209ec6a22942a28c763207c27d503 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/144983581) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4451)
* 3e0617bd355a1e8081ebb9ce615365cb8296dbf2 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/145014752) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4452)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363054887
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
> If some developers happen to create the same config key in different classes, I‘m afraid there exists some risk that user configure that key in flink-conf.yaml but taken effect by another unexpected class.
That's exactly what this check is preventing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on issue #10748: [FLINK-15458][conf][docs]
Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-574702973
I went with the latter approach of removing check `1)`; we have a logically sound argument that the check is redundant, and I prefer that over test-specific fields in the documentation.
Options and entire classes can now be annotated with `Documentation.SuffixOption`. Only if all options for a given key are suffix options they are excluded from the check for being well-defined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r362784610
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
Review comment:
yes I did, happened while cleaning up the history...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r365536998
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,105 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ "host",
+ "port"
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
+ // we allow multiple instances of ConfigOptions with the same key if they are identical
+ return option1;
+ } else {
+ throw duplicateHandler.apply(option1, option2);
+ }
+ }));
+ }
+
+ private static void compareDocumentedAndExistingOptions(Map<String, List<DocumentedOption>> documentedOptions, Map<String, List<ExistingOption>> existingOptions) {
final Collection<String> problems = new ArrayList<>(0);
// first check that all existing options are properly documented
- existingOptions.forEach((key, supposedState) -> {
- DocumentedOption documentedState = documentedOptions.remove(key);
-
- // if nothing matches the docs for this option are up-to-date
- if (documentedState == null) {
- // option is not documented at all
- problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
- } else if (!supposedState.defaultValue.equals(documentedState.defaultValue)) {
- // default is outdated
- problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated. Expected: " + supposedState.defaultValue + " Actual: " + documentedState.defaultValue);
- } else if (!supposedState.description.equals(documentedState.description)) {
- // description is outdated
- problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
- " is outdated.");
+ existingOptions.forEach((key, supposedStates) -> {
+ List<DocumentedOption> documentedState = documentedOptions.remove(key);
+
+ for (ExistingOption supposedState : supposedStates) {
+ if (documentedState == null || documentedState.isEmpty()) {
+ // option is not documented at all
+ problems.add("Option " + supposedState.key + " in " + supposedState.containingClass + " is not documented.");
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.defaultValue.equals(documentedOption.defaultValue))) {
+ // default is outdated
+ problems.add("Documented default of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated. Expected: " + supposedState.defaultValue);
+ } else if (documentedState.stream().noneMatch(documentedOption -> supposedState.description.equals(documentedOption.description))) {
+ // description is outdated
+ problems.add("Documented description of " + supposedState.key + " in " + supposedState.containingClass +
+ " is outdated.");
+ } else {
+ // the docs for this option are up-to-date
+ }
Review comment:
However, something that could indeed slip through if there are say, 3 documented options with key X, but only 2 existing options with that key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/142994287 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:6c2eb4fc84da2322124459c60c8263325cd8b6f1 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081 TriggerType:PUSH TriggerID:6c2eb4fc84da2322124459c60c8263325cd8b6f1
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
Hash:c94dd550a738913273947aca381b5f290d618e81 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/144536250 TriggerType:PUSH TriggerID:c94dd550a738913273947aca381b5f290d618e81
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
* 6c2eb4fc84da2322124459c60c8263325cd8b6f1 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/142994287) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4081)
* c94dd550a738913273947aca381b5f290d618e81 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/144536250) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4372)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#discussion_r363101760
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -61,50 +64,103 @@
private static final Formatter htmlFormatter = new HtmlFormatter();
+ // options for which we allow distinct definitions
+ // this allows reporters to define their own options that are technically only key suffixes
+ private static final Set<String> WELL_DEFINED_WHITELIST = new HashSet<>(Arrays.asList(
+ ));
+
@Test
public void testCommonSectionCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedCommonOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedCommonOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(
optionWithMetaInfo -> optionWithMetaInfo.field.getAnnotation(Documentation.CommonOption.class) != null);
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
+
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
@Test
public void testFullReferenceCompleteness() throws IOException, ClassNotFoundException {
- Map<String, DocumentedOption> documentedOptions = parseDocumentedOptions();
- Map<String, ExistingOption> existingOptions = findExistingOptions(ignored -> true);
+ Map<String, List<DocumentedOption>> documentedOptions = parseDocumentedOptions();
+ Map<String, List<ExistingOption>> existingOptions = findExistingOptions(ignored -> true);
+
+ assertDocumentedOptionsAreWellDefined(documentedOptions);
+ assertExistingOptionsAreWellDefined(existingOptions);
compareDocumentedAndExistingOptions(documentedOptions, existingOptions);
}
- private static void compareDocumentedAndExistingOptions(Map<String, DocumentedOption> documentedOptions, Map<String, ExistingOption> existingOptions) {
+ private static void assertDocumentedOptionsAreWellDefined(Map<String, List<DocumentedOption>> documentedOptions) {
+ assertOptionsAreWellDefined(documentedOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Documentation contains distinct defaults for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ } else {
+ throw new AssertionError("Documentation contains distinct descriptions for " +
+ option1.key + " in " + option1.containingFile + " and " + option2.containingFile + '.');
+ }
+ });
+ }
+
+ private static void assertExistingOptionsAreWellDefined(Map<String, List<ExistingOption>> existingOptions) {
+ assertOptionsAreWellDefined(existingOptions, (option1, option2) -> {
+ // found a ConfigOption pair with the same key that aren't equal
+ // we fail here outright as this is not a documentation-completeness problem
+ if (!option1.defaultValue.equals(option2.defaultValue)) {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct default values (" + option1.defaultValue + " vs " + option2.defaultValue + ").");
+ } else {
+ throw new AssertionError("Ambiguous option " + option1.key + " due to distinct descriptions.");
+ }
+ });
+ }
+
+ private static <O> void assertOptionsAreWellDefined(Map<String, List<O>> allOptions, BiFunction<O, O, AssertionError> duplicateHandler) {
+ allOptions.entrySet().stream()
+ .filter(entry -> !WELL_DEFINED_WHITELIST.contains(entry.getKey()))
+ .map(Map.Entry::getValue)
+ .forEach(options -> options.stream().reduce((option1, option2) -> {
+ if (option1.equals(option2)) {
Review comment:
That's legal. IIRC, when we introduced the generator we actually had an option that was mirroring another one. One could also argue that logically, if code-structure must not have semantics, duplication of code must be allowed.
However, it seems unlikely that 2 options are being created for which they
1. share a key
2. have an _exactly_ matching description (supposedly written in isolation from each other)
3. are semantically so aligned that the key/description are still meaningful
4. yet are semantically so different that the accidental configuration can cause problems.
Overall I think this is mostly a theoretical issue due to us usually providing a component context via the key; to take your example, all `RocksDBOptions` have `rocksdb` in their key, with the inverse rightfully being the case for the `CheckpointingOptions`.
Note that in any case the behavior in this regard isn't different to `master`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #10748:
[FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #10748: [FLINK-15458][conf][docs] Add support for whitelisting ambiguous options
URL: https://github.com/apache/flink/pull/10748#issuecomment-570221103
<!--
Meta data
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://travis-ci.com/flink-ci/flink/builds/142877759 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
Hash:38514c02c79ab0ddb7e80bb483b6034a40d4e199 Status:FAILURE URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047 TriggerType:PUSH TriggerID:38514c02c79ab0ddb7e80bb483b6034a40d4e199
-->
## CI report:
* 38514c02c79ab0ddb7e80bb483b6034a40d4e199 Travis: [FAILURE](https://travis-ci.com/flink-ci/flink/builds/142877759) Azure: [FAILURE](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=4047)
<details>
<summary>Bot commands</summary>
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
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services