You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by andrew-half <gi...@git.apache.org> on 2018/05/10 11:59:23 UTC
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
GitHub user andrew-half opened a pull request:
https://github.com/apache/flink/pull/5985
[FLINK-8135][docs] Add description to MessageParameter
## This pull request adds methods for auto-documenting REST API.
Introduced getDescription() method will generate path and query parameter description on
[REST API documentation](https://ci.apache.org/projects/flink/flink-docs-master/monitoring/rest_api.html) page.
## Brief change log
- An abstract **getDescription()** method was added to the base **MessageParameter.java** class.
- All extending classes overrides the method with brief parameter description.
- The method is called in the **RestAPIDocGenerator.java** class for filling description sections in the output HTML file.
This change is a trivial documentation fix without any test coverage.
## This pull request potentially affect one of the following parts:
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- This pull request just adds documentation for an existed API
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andrew-half/flink FLINK-8135
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5985.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5985
----
commit 5026c42f23837adefbb18eb7421638d72ceef19a
Author: Andrei Poluliakh <an...@...>
Date: 2018-05-10T08:27:23Z
[FLINK-8135][docs] Add description to MessageParameter
----
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187377384
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/rest/messages/MessageParametersTest.java ---
@@ -109,8 +114,13 @@ public JobID convertStringToValue(String value) {
}
@Override
- public String convertValueToString(JobID value) {
- return value.toString();
+ public String convertValueToString(JobID value) {
--- End diff --
revert indentation change
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188603356
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
+ * Null values are spipped.
--- End diff --
typo: skipped
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187939891
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/SubtaskIndexPathParameter.java ---
@@ -44,4 +44,9 @@ protected String convertToString(final Integer value) {
return value.toString();
}
+ @Override
+ public String getDescription() {
+ return "The index of a subtask. Integer value starts from 0, should be positive.";
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187378661
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/ProgramArgsQueryParameter.java ---
@@ -30,4 +30,8 @@ public ProgramArgsQueryParameter() {
super("program-args", MessageParameterRequisiteness.OPTIONAL);
}
+ @Override
+ public String getDescription() {
+ return "Specifies the arguments for the program or plan in \"--foo bar\" format.";
--- End diff --
the format can be arbitrary AFAIK
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188603266
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
--- End diff --
typo: quotes
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by walterddr <gi...@git.apache.org>.
Github user walterddr commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187459062
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/AccumulatorsIncludeSerializedValueQueryParameter.java ---
@@ -38,4 +38,10 @@ public String convertValueToString(Boolean value) {
public Boolean convertStringToValue(String value) {
return Boolean.valueOf(value);
}
+
+ @Override
+ public String getDescription() {
+ return "Boolean value. If true then serialized user task accumulators will be included into a response. " +
+ "False by default.";
--- End diff --
"False by default." is confusing to me, is "false": the default value of the parameter is `false`, or "false": whether the task accumulator will be included or not depends on a "default" configuration from the task accumulator?
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188603431
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
+ * Null values are spipped.
+ *
+ * @param values array of elements for the list
+ *
+ * @return The string with quated list of elements
+ */
+ public static String toQuatedListString(Object[] values) {
--- End diff --
typo: quoted
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187381371
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/MetricsAggregationParameter.java ---
@@ -46,6 +46,13 @@ public String convertValueToString(AggregationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Comma-separated list of aggregates which should be calculated. Available aggregations are " +
+ " \"sum\", \"max\", \"min\" and \"avg\". Unknown aggregations are ignored. " +
--- End diff --
`Unknown aggregations are ignored.` seems unnecessary.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187944451
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/taskmanager/TaskManagerIdPathParameter.java ---
@@ -41,4 +41,9 @@ protected ResourceID convertFromString(String value) {
protected String convertToString(ResourceID value) {
return value.getResourceIdString();
}
+
+ @Override
+ public String getDescription() {
+ return "Task manager ID. Hexadecimal string representation of unique 16 bytes value.";
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by walterddr <gi...@git.apache.org>.
Github user walterddr commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187460147
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/SubtaskIndexPathParameter.java ---
@@ -44,4 +44,9 @@ protected String convertToString(final Integer value) {
return value.toString();
}
+ @Override
+ public String getDescription() {
+ return "The index of a subtask. Integer value starts from 0, should be positive.";
--- End diff --
`must be non-negative`, if "0" is valid
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187940843
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/MetricsAggregationParameter.java ---
@@ -46,6 +46,13 @@ public String convertValueToString(AggregationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Comma-separated list of aggregates which should be calculated. Available aggregations are " +
+ " \"sum\", \"max\", \"min\" and \"avg\". Unknown aggregations are ignored. " +
--- End diff --
In some cases a user should know that mistakes can be hidden without warnings.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by walterddr <gi...@git.apache.org>.
Github user walterddr commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187458764
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/SavepointPathQueryParameter.java ---
@@ -28,4 +28,9 @@
public SavepointPathQueryParameter() {
super(KEY, MessageParameterRequisiteness.OPTIONAL);
}
+
+ @Override
+ public String getDescription() {
+ return "Defines a path to savepoint to restore from.";
--- End diff --
" path to savepoint" -> "path of the savepoint" ?
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187938928
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/TerminationModeQueryParameter.java ---
@@ -41,6 +41,11 @@ public String convertValueToString(TerminationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Defines the termination mode. Supported values are: \"cancel\",\"stop\" (case insensitive). ";
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188604472
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/AllowNonRestoredStateQueryParameter.java ---
@@ -42,4 +42,10 @@ public Boolean convertStringToValue(final String value) {
public String convertValueToString(final Boolean value) {
return value.toString();
}
+
+ @Override
+ public String getDescription() {
+ return "Boolean value that specifies whether non restored state is allowed " +
--- End diff --
double space after "whether"
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188604321
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
+ * Null values are spipped.
+ *
+ * @param values array of elements for the list
+ *
+ * @return The string with quated list of elements
+ */
+ public static String toQuatedListString(Object[] values) {
+ return Arrays.stream(values).filter(Objects::nonNull)
+ .map(v -> v.toString().toLowerCase())
--- End diff --
this reminds me to check whether all enum parameters accept lower-case values...
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187382132
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/taskmanager/TaskManagerIdPathParameter.java ---
@@ -41,4 +41,9 @@ protected ResourceID convertFromString(String value) {
protected String convertToString(ResourceID value) {
return value.getResourceIdString();
}
+
+ @Override
+ public String getDescription() {
+ return "Task manager ID. Hexadecimal string representation of unique 16 bytes value.";
--- End diff --
I prefer `32 character hexadecimal string ...`.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187938237
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/rest/messages/MessageParametersTest.java ---
@@ -109,8 +114,13 @@ public JobID convertStringToValue(String value) {
}
@Override
- public String convertValueToString(JobID value) {
- return value.toString();
+ public String convertValueToString(JobID value) {
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188604517
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/EntryClassQueryParameter.java ---
@@ -28,4 +28,10 @@
public EntryClassQueryParameter() {
super("entry-class", MessageParameterRequisiteness.OPTIONAL);
}
+
+ @Override
+ public String getDescription() {
+ return "String value specifies the fully qualified name of the entry point class. " +
--- End diff --
missing "that" after value, (more occurrences in other files)
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187378292
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/TerminationModeQueryParameter.java ---
@@ -41,6 +41,11 @@ public String convertValueToString(TerminationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Defines the termination mode. Supported values are: \"cancel\",\"stop\" (case insensitive). ";
--- End diff --
derive the available aggregations from `TerminationMode` enum instead.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187944667
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/SavepointPathQueryParameter.java ---
@@ -28,4 +28,9 @@
public SavepointPathQueryParameter() {
super(KEY, MessageParameterRequisiteness.OPTIONAL);
}
+
+ @Override
+ public String getDescription() {
+ return "Defines a path to savepoint to restore from.";
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187938285
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/MetricsAggregationParameter.java ---
@@ -46,6 +46,13 @@ public String convertValueToString(AggregationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Comma-separated list of aggregates which should be calculated. Available aggregations are " +
+ " \"sum\", \"max\", \"min\" and \"avg\". Unknown aggregations are ignored. " +
--- End diff --
Done
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187378111
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/MetricsAggregationParameter.java ---
@@ -46,6 +46,13 @@ public String convertValueToString(AggregationMode value) {
return value.name().toLowerCase();
}
+ @Override
+ public String getDescription() {
+ return "Comma-separated list of aggregates which should be calculated. Available aggregations are " +
+ " \"sum\", \"max\", \"min\" and \"avg\". Unknown aggregations are ignored. " +
--- End diff --
derive the available aggregations from `AggregationMode` enum instead.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187941477
--- Diff: flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/ProgramArgsQueryParameter.java ---
@@ -30,4 +30,8 @@ public ProgramArgsQueryParameter() {
super("program-args", MessageParameterRequisiteness.OPTIONAL);
}
+ @Override
+ public String getDescription() {
+ return "Specifies the arguments for the program or plan in \"--foo bar\" format.";
--- End diff --
Added examples from JavaDoc. May be you could provide me a batter description?
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188603310
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
--- End diff --
typo: returned
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187945098
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/AccumulatorsIncludeSerializedValueQueryParameter.java ---
@@ -38,4 +38,10 @@ public String convertValueToString(Boolean value) {
public Boolean convertStringToValue(String value) {
return Boolean.valueOf(value);
}
+
+ @Override
+ public String getDescription() {
+ return "Boolean value. If true then serialized user task accumulators will be included into a response. " +
+ "False by default.";
--- End diff --
Done
---
[GitHub] flink issue #5985: [FLINK-8135][docs] Add description to MessageParameter
Posted by andrew-half <gi...@git.apache.org>.
Github user andrew-half commented on the issue:
https://github.com/apache/flink/pull/5985
@walterddr @zentol Thank you guys for a substantial code-review. Please check the updates then you have time.
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r187378407
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/SubtaskIndexPathParameter.java ---
@@ -44,4 +44,9 @@ protected String convertToString(final Integer value) {
return value.toString();
}
+ @Override
+ public String getDescription() {
+ return "The index of a subtask. Integer value starts from 0, should be positive.";
--- End diff --
_must_ be positive, same comment applies to other parameters
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188604063
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
+ * Null values are spipped.
+ *
+ * @param values array of elements for the list
+ *
+ * @return The string with quated list of elements
+ */
+ public static String toQuatedListString(Object[] values) {
--- End diff --
We could move this into the `Utils´ class in `flink-docs`, at the moment I don't see much other use for it..
---
[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5985#discussion_r188603410
--- Diff: flink-core/src/main/java/org/apache/flink/util/StringUtils.java ---
@@ -348,6 +350,21 @@ public static String concatenateWithAnd(@Nullable String s1, @Nullable String s2
}
}
+ /**
+ * Generates a string containing a comma-separated list of values in double-quatas.
+ * Uses lower-cased values returning from {@link Object#toString()} method for each element in the given array.
+ * Null values are spipped.
+ *
+ * @param values array of elements for the list
+ *
+ * @return The string with quated list of elements
--- End diff --
typo: quoted
---