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

Remove or improve (not set) text in the Job Plan UI

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.0, 1.1.4
    • Component/s: Webfrontend
    • Labels:
      None

      Description

      When running streaming jobs the UI display (not set) in the UI in a few different places. This is not the case for batch jobs.

      To illustrate I've included screen shots of the UI for the batch and streaming WordCount examples.

        Issue Links

          Activity

          Hide
          ivan.mushketyk Ivan Mushketyk added a comment -

          I'll work on this.

          Show
          ivan.mushketyk Ivan Mushketyk added a comment - I'll work on this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user mushketyk opened a pull request:

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

          FLINK-3680 Remove "(not set)" text in the Job Plan UI

          This PR removes the "(not set)" text in the web frontend.

          Currently it looks like this: https://issues.apache.org/jira/secure/attachment/12796006/Screen%20Shot%202016-03-29%20at%208.13.12%20PM.png

          Now it looks like:
          ![fixed labels](https://cloud.githubusercontent.com/assets/592286/18181550/815897c6-7083-11e6-8bbe-9825ac144c83.png)

          • [x] General
          • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
          • The pull request addresses only one issue
          • Each commit in the PR has a meaningful commit message (including the JIRA id)
          • [x] Documentation
          • Documentation has been added for new functionality
          • Old documentation affected by the pull request has been updated
          • JavaDoc for public methods has been added
          • [x] Tests & Build
          • Functionality added by the pull request is covered by tests
          • `mvn clean verify` has been executed successfully locally or a Travis build has passed

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

          $ git pull https://github.com/mushketyk/flink improve-text

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

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


          commit 67db4e7c03787645c45ef3bc95b45060892bb569
          Author: Ivan Mushketyk <ivan.mushketik@gmail.com>
          Date: 2016-09-01T19:24:29Z

          FLINK-3680 Remove "(not set)" text in the Job Plan UI


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user mushketyk opened a pull request: https://github.com/apache/flink/pull/2457 FLINK-3680 Remove "(not set)" text in the Job Plan UI This PR removes the "(not set)" text in the web frontend. Currently it looks like this: https://issues.apache.org/jira/secure/attachment/12796006/Screen%20Shot%202016-03-29%20at%208.13.12%20PM.png Now it looks like: ! [fixed labels] ( https://cloud.githubusercontent.com/assets/592286/18181550/815897c6-7083-11e6-8bbe-9825ac144c83.png ) [x] General The pull request references the related JIRA issue (" [FLINK-XXX] Jira title text") The pull request addresses only one issue Each commit in the PR has a meaningful commit message (including the JIRA id) [x] Documentation Documentation has been added for new functionality Old documentation affected by the pull request has been updated JavaDoc for public methods has been added [x] Tests & Build Functionality added by the pull request is covered by tests `mvn clean verify` has been executed successfully locally or a Travis build has passed You can merge this pull request into a Git repository by running: $ git pull https://github.com/mushketyk/flink improve-text Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2457.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 #2457 commit 67db4e7c03787645c45ef3bc95b45060892bb569 Author: Ivan Mushketyk <ivan.mushketik@gmail.com> Date: 2016-09-01T19:24:29Z FLINK-3680 Remove "(not set)" text in the Job Plan UI
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mushketyk commented on the issue:

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

          @iampeter
          Did you have a chance to take a look at this change?

          Show
          githubbot ASF GitHub Bot added a comment - Github user mushketyk commented on the issue: https://github.com/apache/flink/pull/2457 @iampeter Did you have a chance to take a look at this change?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/2457#discussion_r87440964

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/jsonplan/JsonPlanGenerator.java —
          @@ -52,10 +52,10 @@ public static String generatePlan(JobGraph jg) {
          for (JobVertex vertex : jg.getVertices()) {

          String operator = vertex.getOperatorName() != null ?

          • vertex.getOperatorName() : NOT_SET;
            + vertex.getOperatorName() : "";
              • End diff –

          Why not change `NOT_SET` to be `""`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on a diff in the pull request: https://github.com/apache/flink/pull/2457#discussion_r87440964 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/jsonplan/JsonPlanGenerator.java — @@ -52,10 +52,10 @@ public static String generatePlan(JobGraph jg) { for (JobVertex vertex : jg.getVertices()) { String operator = vertex.getOperatorName() != null ? vertex.getOperatorName() : NOT_SET; + vertex.getOperatorName() : ""; End diff – Why not change `NOT_SET` to be `""`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mushketyk commented on the issue:

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

          Hi @greghogan
          I've fixed the code according to your suggestion.
          Could you give it another look?

          Show
          githubbot ASF GitHub Bot added a comment - Github user mushketyk commented on the issue: https://github.com/apache/flink/pull/2457 Hi @greghogan I've fixed the code according to your suggestion. Could you give it another look?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/2457#discussion_r87659296

          — Diff: flink-runtime-web/web-dashboard/app/scripts/modules/jobs/jobs.dir.coffee —
          @@ -281,7 +281,8 @@ angular.module('flinkApp')

          1. Otherwise add infos
            labelValue += "<h5>" + info + " Node</h5>" if isSpecialIterationNode(info)
            labelValue += "<h5>Parallelism: " + el.parallelism + "</h5>" unless el.parallelism is ""
          • labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined`
            + if el.operator_strategy
            + labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined`
              • End diff –

          It's fine, but maybe:
          ```
          labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless !el.operator_strategy || el.operator is `undefined`
          ```

          doesn't really matter much, can be as well as you've put it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user iampeter commented on a diff in the pull request: https://github.com/apache/flink/pull/2457#discussion_r87659296 — Diff: flink-runtime-web/web-dashboard/app/scripts/modules/jobs/jobs.dir.coffee — @@ -281,7 +281,8 @@ angular.module('flinkApp') Otherwise add infos labelValue += "<h5>" + info + " Node</h5>" if isSpecialIterationNode(info) labelValue += "<h5>Parallelism: " + el.parallelism + "</h5>" unless el.parallelism is "" labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined` + if el.operator_strategy + labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined` End diff – It's fine, but maybe: ``` labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless !el.operator_strategy || el.operator is `undefined` ``` doesn't really matter much, can be as well as you've put it.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/2457#discussion_r87701980

          — Diff: flink-runtime-web/web-dashboard/app/scripts/modules/jobs/jobs.dir.coffee —
          @@ -281,7 +281,8 @@ angular.module('flinkApp')

          1. Otherwise add infos
            labelValue += "<h5>" + info + " Node</h5>" if isSpecialIterationNode(info)
            labelValue += "<h5>Parallelism: " + el.parallelism + "</h5>" unless el.parallelism is ""
          • labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined`
            + if el.operator_strategy
            + labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined`
              • End diff –

          I think this will make it more consistent. I'll update it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user mushketyk commented on a diff in the pull request: https://github.com/apache/flink/pull/2457#discussion_r87701980 — Diff: flink-runtime-web/web-dashboard/app/scripts/modules/jobs/jobs.dir.coffee — @@ -281,7 +281,8 @@ angular.module('flinkApp') Otherwise add infos labelValue += "<h5>" + info + " Node</h5>" if isSpecialIterationNode(info) labelValue += "<h5>Parallelism: " + el.parallelism + "</h5>" unless el.parallelism is "" labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined` + if el.operator_strategy + labelValue += "<h5>Operation: " + shortenString(el.operator_strategy) + "</h5>" unless el.operator is `undefined` End diff – I think this will make it more consistent. I'll update it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mushketyk commented on the issue:

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

          Hi @iampeter
          I've updated the PR according to you suggestion.

          Show
          githubbot ASF GitHub Bot added a comment - Github user mushketyk commented on the issue: https://github.com/apache/flink/pull/2457 Hi @iampeter I've updated the PR according to you suggestion.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user iampeter commented on the issue:

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

          @mushketyk from my side looks good

          Show
          githubbot ASF GitHub Bot added a comment - Github user iampeter commented on the issue: https://github.com/apache/flink/pull/2457 @mushketyk from my side looks good
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

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

          1) `JsonPlanGenerator.java:55` should be reverted to use `NOT_SET`.
          2) `index.js` is changing jobServer from the empty string to 'http://localhost:8081/'.
          3) `vendor.js` is upgrading the version of `imagesLoaded` from 4.1.0 to 4.1.1. In the past we have specified in `flink-runtime-web/web-dashboard/bower.json` to not upgrade point releases since the changes were obfuscating the remainder of the pull request. I have not noticed if we have subsequently followed up with a PR to upgrade the libraries. We may be at a good point in the 1.2 release cycle to do this now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/2457 1) `JsonPlanGenerator.java:55` should be reverted to use `NOT_SET`. 2) `index.js` is changing jobServer from the empty string to 'http://localhost:8081/'. 3) `vendor.js` is upgrading the version of `imagesLoaded` from 4.1.0 to 4.1.1. In the past we have specified in `flink-runtime-web/web-dashboard/bower.json` to not upgrade point releases since the changes were obfuscating the remainder of the pull request. I have not noticed if we have subsequently followed up with a PR to upgrade the libraries. We may be at a good point in the 1.2 release cycle to do this now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user iampeter commented on the issue:

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

          @greghogan right - missed the generated files. @mushketyk I suppose `gulp`ing will help

          Show
          githubbot ASF GitHub Bot added a comment - Github user iampeter commented on the issue: https://github.com/apache/flink/pull/2457 @greghogan right - missed the generated files. @mushketyk I suppose `gulp`ing will help
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user mushketyk commented on the issue:

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

          @iampeter @greghogan I've update the PR according to your review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user mushketyk commented on the issue: https://github.com/apache/flink/pull/2457 @iampeter @greghogan I've update the PR according to your review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uce commented on the issue:

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

          Would be cool if we could also merge this to `release-1.1` if it is good to merge.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on the issue: https://github.com/apache/flink/pull/2457 Would be cool if we could also merge this to `release-1.1` if it is good to merge.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user greghogan commented on the issue:

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

          Merging this ...

          Show
          githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/2457 Merging this ...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/2457
          Hide
          greghogan Greg Hogan added a comment -

          1.1: 7b5d769aded2b653ac4e573bf0d9c63e250ef684
          1.2: 16c08b54c4cce8801981eb6d4c79cf972b5e85b6

          Show
          greghogan Greg Hogan added a comment - 1.1: 7b5d769aded2b653ac4e573bf0d9c63e250ef684 1.2: 16c08b54c4cce8801981eb6d4c79cf972b5e85b6

            People

            • Assignee:
              ivan.mushketyk Ivan Mushketyk
              Reporter:
              jgrier Jamie Grier
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development