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:
us...@infra.apache.org


Reply via email to