Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6581

Dynamic property parsing broken for YARN

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.0, 1.4.0
    • Fix Version/s: 1.3.0, 1.2.2, 1.4.0
    • Component/s: YARN
    • Labels:
      None

      Description

      The dynamic property parsing in YARN is broken. For example, the following dynamic property won't be parsed properly: -yDenv.java.opts="-DappName=foobar -Did=1". The result will be equivalent to -yDenv.java.opts="-DappName".

      The problem is that FlinkYarnSessionCli#getDynamicProperties assumes that all dynamic properties have the form -D<KEY>=<VALUE> where <VALUE> does not contain any =.

      I propose to change the parsing of the dynamic properties string such that it splits the key-value pair not via dynamicProperties.split("=") but that we split the dynamic properties string at the first occurrence of the =.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user tillrohrmann opened a pull request:

          https://github.com/apache/flink/pull/3903

          FLINK-6581 [cli] Correct dynamic property parsing for YARN cli

          The YARN cli will now split the dynamic propertie at the first occurrence of
          the = sign instead of splitting it at every = sign. That way we support dynamic
          properties of the form -yDenv.java.opts="-DappName=foobar".

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/tillrohrmann/flink fixYarnDynPropertyParsing

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3903.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 #3903


          commit 5988d06fd5bc8e6a20e924bb009aaf3f9b5f884b
          Author: Till Rohrmann <trohrmann@apache.org>
          Date: 2017-05-15T10:04:07Z

          FLINK-6581 [cli] Correct dynamic property parsing for YARN cli

          The YARN cli will now split the dynamic propertie at the first occurrence of
          the = sign instead of splitting it at every = sign. That way we support dynamic
          properties of the form -yDenv.java.opts="-DappName=foobar".


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user tillrohrmann opened a pull request: https://github.com/apache/flink/pull/3903 FLINK-6581 [cli] Correct dynamic property parsing for YARN cli The YARN cli will now split the dynamic propertie at the first occurrence of the = sign instead of splitting it at every = sign. That way we support dynamic properties of the form -yDenv.java.opts="-DappName=foobar". You can merge this pull request into a Git repository by running: $ git pull https://github.com/tillrohrmann/flink fixYarnDynPropertyParsing Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3903.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 #3903 commit 5988d06fd5bc8e6a20e924bb009aaf3f9b5f884b Author: Till Rohrmann <trohrmann@apache.org> Date: 2017-05-15T10:04:07Z FLINK-6581 [cli] Correct dynamic property parsing for YARN cli The YARN cli will now split the dynamic propertie at the first occurrence of the = sign instead of splitting it at every = sign. That way we support dynamic properties of the form -yDenv.java.opts="-DappName=foobar".
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3903#discussion_r116485611

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java —
          @@ -702,9 +702,15 @@ private void logAndSysout(String message)

          { continue; }
          • String[] kv = propLine.split("=");
          • if (kv.length >= 2 && kv[0] != null && kv[1] != null && kv[0].length() > 0) {
          • properties.put(kv[0], kv[1]);
            + int firstEquals = propLine.indexOf("=");
            +
            + if (firstEquals >= 0) {
            + String key = propLine.substring(0, firstEquals).trim();
            + String value = propLine.substring(firstEquals + 1, propLine.length()).trim();
            +
            + if (!key.isEmpty() && !value.isEmpty()) {
              • End diff –

          We didn't reject empty values before. Not sure if there's a valid use-case for that though, maybe overriding/clearing some variable?

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3903#discussion_r116485611 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java — @@ -702,9 +702,15 @@ private void logAndSysout(String message) { continue; } String[] kv = propLine.split("="); if (kv.length >= 2 && kv [0] != null && kv [1] != null && kv [0] .length() > 0) { properties.put(kv [0] , kv [1] ); + int firstEquals = propLine.indexOf("="); + + if (firstEquals >= 0) { + String key = propLine.substring(0, firstEquals).trim(); + String value = propLine.substring(firstEquals + 1, propLine.length()).trim(); + + if (!key.isEmpty() && !value.isEmpty()) { End diff – We didn't reject empty values before. Not sure if there's a valid use-case for that though, maybe overriding/clearing some variable?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3903#discussion_r116494377

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java —
          @@ -702,9 +702,15 @@ private void logAndSysout(String message)

          { continue; }
          • String[] kv = propLine.split("=");
          • if (kv.length >= 2 && kv[0] != null && kv[1] != null && kv[0].length() > 0) {
          • properties.put(kv[0], kv[1]);
            + int firstEquals = propLine.indexOf("=");
            +
            + if (firstEquals >= 0) {
            + String key = propLine.substring(0, firstEquals).trim();
            + String value = propLine.substring(firstEquals + 1, propLine.length()).trim();
            +
            + if (!key.isEmpty() && !value.isEmpty()) {
              • End diff –

          True, in order to keep in sync with the old behaviour, I'll remove the check whether a value is empty.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/3903#discussion_r116494377 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java — @@ -702,9 +702,15 @@ private void logAndSysout(String message) { continue; } String[] kv = propLine.split("="); if (kv.length >= 2 && kv [0] != null && kv [1] != null && kv [0] .length() > 0) { properties.put(kv [0] , kv [1] ); + int firstEquals = propLine.indexOf("="); + + if (firstEquals >= 0) { + String key = propLine.substring(0, firstEquals).trim(); + String value = propLine.substring(firstEquals + 1, propLine.length()).trim(); + + if (!key.isEmpty() && !value.isEmpty()) { End diff – True, in order to keep in sync with the old behaviour, I'll remove the check whether a value is empty.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3903

          Thanks for the review @zentol. I'll address your comment and then merge the PR once Travis gives green light.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3903 Thanks for the review @zentol. I'll address your comment and then merge the PR once Travis gives green light.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rmetzger commented on the issue:

          https://github.com/apache/flink/pull/3903

          +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/3903 +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user johnboy14 commented on the issue:

          https://github.com/apache/flink/pull/3903

          Thanks for turning this around quickly @tillrohrmann We are experiencing this issue with AWS EMR. Is it possible to patch the current 1.2.0 version available via EMR?

          Show
          githubbot ASF GitHub Bot added a comment - Github user johnboy14 commented on the issue: https://github.com/apache/flink/pull/3903 Thanks for turning this around quickly @tillrohrmann We are experiencing this issue with AWS EMR. Is it possible to patch the current 1.2.0 version available via EMR?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3903

          @johnboy14 I can merge this PR also into the `1.2` branch. This means that it will be included in the next bug fix release `1.2.2`. How quickly it will be included in EMR depends a little bit on the AWS/Apache BigTop. We can try to ping them. But if customers complain, then it's much more efficient.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3903 @johnboy14 I can merge this PR also into the `1.2` branch. This means that it will be included in the next bug fix release `1.2.2`. How quickly it will be included in EMR depends a little bit on the AWS/Apache BigTop. We can try to ping them. But if customers complain, then it's much more efficient.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user johnboy14 commented on the issue:

          https://github.com/apache/flink/pull/3903

          When's the next bug fix release scheduled for @tillrohrmann ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user johnboy14 commented on the issue: https://github.com/apache/flink/pull/3903 When's the next bug fix release scheduled for @tillrohrmann ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tillrohrmann commented on the issue:

          https://github.com/apache/flink/pull/3903

          This is not decided yet. Usually we do it depending on the urgency of the fixes and the time since the last bug fix release. Currently, we're working on the `1.3` release which requires most of our time. Thus, I would assume not before mid of next month but this is a community decision.

          Show
          githubbot ASF GitHub Bot added a comment - Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/3903 This is not decided yet. Usually we do it depending on the urgency of the fixes and the time since the last bug fix release. Currently, we're working on the `1.3` release which requires most of our time. Thus, I would assume not before mid of next month but this is a community decision.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user johnboy14 commented on the issue:

          https://github.com/apache/flink/pull/3903

          At this time I am looking a workaround in the meantime. Is there anyway to override the log4j.properties file for each application deployed to EMR?

          Show
          githubbot ASF GitHub Bot added a comment - Github user johnboy14 commented on the issue: https://github.com/apache/flink/pull/3903 At this time I am looking a workaround in the meantime. Is there anyway to override the log4j.properties file for each application deployed to EMR?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3903

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3903
          Hide
          till.rohrmann Till Rohrmann added a comment -

          1.4.0: dceb5cc1759bc091a2f3a6e360771c404b87f7f4
          1.3.0: 4104409cc7a595c17d474333fe0516a3a98a130e
          1.2.2: 1ca57368a8ef241930290930f9264f062cfb2d35

          Show
          till.rohrmann Till Rohrmann added a comment - 1.4.0: dceb5cc1759bc091a2f3a6e360771c404b87f7f4 1.3.0: 4104409cc7a595c17d474333fe0516a3a98a130e 1.2.2: 1ca57368a8ef241930290930f9264f062cfb2d35

            People

            • Assignee:
              till.rohrmann Till Rohrmann
              Reporter:
              till.rohrmann Till Rohrmann
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development