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