[GitHub] [flink] tillrohrmann commented on a change in pull request #13747: [FLINK-19662][runtime] Exposing the JobManager's memory configuration through /jobmanager/config

[GitBox <gi...@apache.org>]

tillrohrmann commented on a change in pull request #13747:
URL: https://github.com/apache/flink/pull/13747#discussion_r510950775



##########
File path: flink-dist/src/main/flink-bin/bin/jobmanager.sh
##########
@@ -51,6 +51,10 @@ if [[ $STARTSTOP == "start" ]] || [[ $STARTSTOP == "start-foreground" ]]; then
         args+=("--webui-port")
         args+=("${WEBUIPORT}")
     fi
+
+    if [ ! -z "${DYNAMIC_PARAMETERS+x}" ]; then

Review comment:
       Why is a `+x` appended to `DYNAMIC_PARAMETERS`? In the other scripts this suffix is missing.

##########
File path: flink-dist/src/main/flink-bin/bin/jobmanager.sh
##########
@@ -51,6 +51,10 @@ if [[ $STARTSTOP == "start" ]] || [[ $STARTSTOP == "start-foreground" ]]; then
         args+=("--webui-port")
         args+=("${WEBUIPORT}")
     fi
+
+    if [ ! -z "${DYNAMIC_PARAMETERS+x}" ]; then
+        for v in ${DYNAMIC_PARAMETERS[@]}; do args+=($v); done

Review comment:
       Why do we have to add it in a loop to `args`? Is this some bash magic?

##########
File path: flink-dist/src/test/java/org/apache/flink/dist/BashJavaUtilsITCase.java
##########
@@ -72,20 +79,23 @@ public void testGetTmResourceParamsConfigsWithDynamicProperties() throws Excepti
 
 	@Test
 	public void testGetJmResourceParams() throws Exception {
-		int expectedResultLines = 1;
+		int expectedResultLines = 2;

Review comment:
       I guess that this commit should be merged together with the previous one because w/o, the previous commit should fail in this test 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