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

yarnClient should be closed in AbstractYarnClusterDescriptor for error conditions

    Details

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

      Description

      Here is one example:

          if(jobManagerMemoryMb > maxRes.getMemory() ) {
            failSessionDuringDeployment(yarnClient, yarnApplication);
            throw new YarnDeploymentException("The cluster does not have the requested resources for the JobManager available!\n"
              + "Maximum Memory: " + maxRes.getMemory() + "MB Requested: " + jobManagerMemoryMb + "MB. " + NOTE);
          }
      

      yarnClient implements Closeable.
      It should be closed in situations where exception is thrown.

        Issue Links

          Activity

          Hide
          Zentol Chesnay Schepler added a comment -

          1.3: 17c5de5a35b4fa72e3636539dfec2cd9135ef91d
          1.4: 3d2f3f65f6d0722a3b93565846f31a5dd28218d1

          Show
          Zentol Chesnay Schepler added a comment - 1.3: 17c5de5a35b4fa72e3636539dfec2cd9135ef91d 1.4: 3d2f3f65f6d0722a3b93565846f31a5dd28218d1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user zentol commented on the issue:

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

          merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/4022 merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/4022#discussion_r123425557

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterClientV2.java —
          @@ -146,7 +146,9 @@ public ApplicationStatus getApplicationStatus() {

          @Override
          public void finalizeCluster() {

          • // Do nothing
              • End diff –

          I have reverted the changes and fixed another problem as Ted Yu metioned in FLINK-5488, thanks @zentol

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjureel commented on a diff in the pull request: https://github.com/apache/flink/pull/4022#discussion_r123425557 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterClientV2.java — @@ -146,7 +146,9 @@ public ApplicationStatus getApplicationStatus() { @Override public void finalizeCluster() { // Do nothing End diff – I have reverted the changes and fixed another problem as Ted Yu metioned in FLINK-5488 , thanks @zentol
          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/4022#discussion_r122688461

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterClientV2.java —
          @@ -146,7 +146,9 @@ public ApplicationStatus getApplicationStatus() {

          @Override
          public void finalizeCluster() {

          • // Do nothing
              • End diff –

          Please revert the changes here. This file is part of another big initiative (referred to as FLIP-6); we shouldn't touch it unless necessary. (basically, I don't know about the yarn-client life-cycle here so let's be conservative.)

          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/4022#discussion_r122688461 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterClientV2.java — @@ -146,7 +146,9 @@ public ApplicationStatus getApplicationStatus() { @Override public void finalizeCluster() { // Do nothing End diff – Please revert the changes here. This file is part of another big initiative (referred to as FLIP-6); we shouldn't touch it unless necessary. (basically, I don't know about the yarn-client life-cycle here so let's be conservative.)
          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/4022#discussion_r122688150

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java —
          @@ -420,6 +421,9 @@ public YarnClusterClient retrieve(String applicationID)

          { return createYarnClusterClient(this, yarnClient, appReport, flinkConfiguration, false); }

          catch (Exception e) {
          + if(null != yarnClient) {
          — End diff –

          missing space after if.

          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/4022#discussion_r122688150 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java — @@ -420,6 +421,9 @@ public YarnClusterClient retrieve(String applicationID) { return createYarnClusterClient(this, yarnClient, appReport, flinkConfiguration, false); } catch (Exception e) { + if(null != yarnClient) { — End diff – missing space after if.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment - - edited

          Please also fix the following in deployInternal() method:

              ClusterResourceDescription freeClusterMem = getCurrentFreeClusterResources(yarnClient);
          
          Show
          yuzhihong@gmail.com Ted Yu added a comment - - edited Please also fix the following in deployInternal() method: ClusterResourceDescription freeClusterMem = getCurrentFreeClusterResources(yarnClient);
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/4022#discussion_r119779957

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java —
          @@ -392,6 +392,7 @@ protected YarnClient getYarnClient() {
          @Override
          public YarnClusterClient retrieve(String applicationID) {

          + final YarnClient yarnClient = getYarnClient();
          try {
          // check if required Hadoop environment variables are set. If not, warn user
          if (System.getenv("HADOOP_CONF_DIR") == null &&
          — End diff –

          It's a good point, I will put `getYarnClient()` in the `try` block

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjureel commented on a diff in the pull request: https://github.com/apache/flink/pull/4022#discussion_r119779957 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java — @@ -392,6 +392,7 @@ protected YarnClient getYarnClient() { @Override public YarnClusterClient retrieve(String applicationID) { + final YarnClient yarnClient = getYarnClient(); try { // check if required Hadoop environment variables are set. If not, warn user if (System.getenv("HADOOP_CONF_DIR") == null && — End diff – It's a good point, I will put `getYarnClient()` in the `try` block
          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/4022#discussion_r119448300

          — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java —
          @@ -392,6 +392,7 @@ protected YarnClient getYarnClient() {
          @Override
          public YarnClusterClient retrieve(String applicationID) {

          + final YarnClient yarnClient = getYarnClient();
          try {
          // check if required Hadoop environment variables are set. If not, warn user
          if (System.getenv("HADOOP_CONF_DIR") == null &&
          — End diff –

          does `getYarnClient()` fail if this condition is fulfilled? If so we should put this if block before the call to `getYarnClient()`.

          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/4022#discussion_r119448300 — Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java — @@ -392,6 +392,7 @@ protected YarnClient getYarnClient() { @Override public YarnClusterClient retrieve(String applicationID) { + final YarnClient yarnClient = getYarnClient(); try { // check if required Hadoop environment variables are set. If not, warn user if (System.getenv("HADOOP_CONF_DIR") == null && — End diff – does `getYarnClient()` fail if this condition is fulfilled? If so we should put this if block before the call to `getYarnClient()`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fanyon closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user fanyon closed the pull request at: https://github.com/apache/flink/pull/3805
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zjureel commented on the issue:

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

          @tzulitai I have make a new PR https://github.com/apache/flink/pull/4022 for this issue and close this PR soon, thanks

          Show
          githubbot ASF GitHub Bot added a comment - Github user zjureel commented on the issue: https://github.com/apache/flink/pull/3805 @tzulitai I have make a new PR https://github.com/apache/flink/pull/4022 for this issue and close this PR soon, thanks
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zjureel opened a pull request:

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

          FLINK-5488 stop YarnClient before exception is thrown

          Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
          If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
          In addition to going through the list, please provide a meaningful description of your changes.

          • [ ] 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)
          • [ ] 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
          • [ ] 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/zjureel/flink FLINK-5488

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

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


          commit c8cd1be559e1da7f0c5978206897e8ac63480765
          Author: zjureel <zjureel@gmail.com>
          Date: 2017-05-31T05:13:34Z

          FLINK-5488 stop YarnClient before exception is thrown


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zjureel opened a pull request: https://github.com/apache/flink/pull/4022 FLINK-5488 stop YarnClient before exception is thrown Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration. If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide] ( http://flink.apache.org/how-to-contribute.html ). In addition to going through the list, please provide a meaningful description of your changes. [ ] 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) [ ] 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 [ ] 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/zjureel/flink FLINK-5488 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/4022.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 #4022 commit c8cd1be559e1da7f0c5978206897e8ac63480765 Author: zjureel <zjureel@gmail.com> Date: 2017-05-31T05:13:34Z FLINK-5488 stop YarnClient before exception is thrown
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user tzulitai commented on the issue:

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

          @fanyon could you rebase this PR?

          Show
          githubbot ASF GitHub Bot added a comment - Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/3805 @fanyon could you rebase this PR?
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          I agree.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - I agree.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fanyon commented on the issue:

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

          In `YarnClusterClientV2`, I think yarn client should be stopped in `finalizeCluster()`, which is empty right now

          Show
          githubbot ASF GitHub Bot added a comment - Github user fanyon commented on the issue: https://github.com/apache/flink/pull/3805 In `YarnClusterClientV2`, I think yarn client should be stopped in `finalizeCluster()`, which is empty right now
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

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

          This looks like something that should be done in a `finally` block.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3805 This looks like something that should be done in a `finally` block.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user fanyon opened a pull request:

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

          FLINK-5488 stop YarnClient before exception is thrown

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

          $ git pull https://github.com/fanyon/flink FLINK-5488

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

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


          commit 2205f7782650a1031d1284f75691966646784c31
          Author: mengji.fy <mengji.fy@taobao.com>
          Date: 2017-05-02T09:10:44Z

          FLINK-5488 stop YarnClient before exception is thrown


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user fanyon opened a pull request: https://github.com/apache/flink/pull/3805 FLINK-5488 stop YarnClient before exception is thrown You can merge this pull request into a Git repository by running: $ git pull https://github.com/fanyon/flink FLINK-5488 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3805.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 #3805 commit 2205f7782650a1031d1284f75691966646784c31 Author: mengji.fy <mengji.fy@taobao.com> Date: 2017-05-02T09:10:44Z FLINK-5488 stop YarnClient before exception is thrown
          Hide
          zjureel Fang Yong added a comment -

          It's good to release resources before exception is thown, I'd create a PR to fix them, thanks

          Show
          zjureel Fang Yong added a comment - It's good to release resources before exception is thown, I'd create a PR to fix them, thanks
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Here is another example from AbstractYarnClusterDescriptor#retrieve() :

                return createYarnClusterClient(this, yarnClient, appReport, flinkConfiguration, sessionFilesDir, false);
              } catch (Exception e) {
                throw new RuntimeException("Couldn't retrieve Yarn cluster", e);
          
          Show
          yuzhihong@gmail.com Ted Yu added a comment - Here is another example from AbstractYarnClusterDescriptor#retrieve() : return createYarnClusterClient( this , yarnClient, appReport, flinkConfiguration, sessionFilesDir, false ); } catch (Exception e) { throw new RuntimeException( "Couldn't retrieve Yarn cluster" , e);

            People

            • Assignee:
              zjureel Fang Yong
              Reporter:
              yuzhihong@gmail.com Ted Yu
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development