[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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


---

[GitHub] flink pull request #5985: [FLINK-8135][docs] Add description to MessageParam...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[andrew-half]

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 pull request #5985: [FLINK-8135][docs] Add description to MessageParam...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[andrew-half]

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...

[walterddr]

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...

[walterddr]

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...

[walterddr]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[zentol]

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...

[andrew-half]

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 
Date:   2018-05-10T08:27:23Z

[FLINK-8135][docs] Add description to MessageParameter




---