Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-175

Hadoop21YarnAppClient caches a YarnClient

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0-incubating
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:
      None

      Description

      Hadoop21YarnAppClient caches a YarnClient.
      Internally, YarnClient is only useful for a single UserGroupInformation. Because of this, you can not use the same Hadoop21YarnAppClient for multiple UGIs.
      The Hadoop21YarnAppClient class should use a different YarnClient for each UGI.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user anwar6953 opened a pull request:

          https://github.com/apache/twill/pull/6

          Avoid caching YarnClient

          Avoid caching a YarnClient, since each YarnClient is fixed to a particular UserGroupInformation.

          https://issues.apache.org/jira/browse/TWILL-175

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

          $ git pull https://github.com/anwar6953/twill feature/avoid-caching-yarn-client

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

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


          commit 4726e409d7b344b92ab7fee881a47aa37fc542dd
          Author: Ali Anwar <anwar1@berkeley.edu>
          Date: 2016-08-26T06:45:39Z

          Avoid caching a YarnClient, since each YarnClient is fixed to a particular UserGroupInformation.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user anwar6953 opened a pull request: https://github.com/apache/twill/pull/6 Avoid caching YarnClient Avoid caching a YarnClient, since each YarnClient is fixed to a particular UserGroupInformation. https://issues.apache.org/jira/browse/TWILL-175 You can merge this pull request into a Git repository by running: $ git pull https://github.com/anwar6953/twill feature/avoid-caching-yarn-client Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/6.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 #6 commit 4726e409d7b344b92ab7fee881a47aa37fc542dd Author: Ali Anwar <anwar1@berkeley.edu> Date: 2016-08-26T06:45:39Z Avoid caching a YarnClient, since each YarnClient is fixed to a particular UserGroupInformation.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76378304

          — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java —
          @@ -76,6 +76,11 @@ public String toString() {

          return new ProcessController<R>() {
          @Override
          + public void close() throws Exception {
          + // no-op
          — End diff –

          Controller ties to the lifecycle of the stuff that it is controlling. What do mean by leave the runnable process running when closing the controller? Is it related to #4 ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76378304 — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java — @@ -76,6 +76,11 @@ public String toString() { return new ProcessController<R>() { @Override + public void close() throws Exception { + // no-op — End diff – Controller ties to the lifecycle of the stuff that it is controlling. What do mean by leave the runnable process running when closing the controller? Is it related to #4 ?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76378540

          — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java —
          @@ -76,6 +76,11 @@ public String toString() {

          return new ProcessController<R>() {
          @Override
          + public void close() throws Exception {
          + // no-op
          — End diff –

          I just meant, now both close and cancel will kill the runnable?

          On Aug 26, 2016 12:55 AM, "Terence Yim" <notifications@github.com> wrote:

          > In twill-yarn/src/main/java/org/apache/twill/internal/appmaster/
          > RunnableProcessLauncher.java
          > <https://github.com/apache/twill/pull/6#discussion_r76378304>:
          >
          > > @@ -76,6 +76,11 @@ public String toString() {
          > >
          > > return new ProcessController<R>() {
          > > @Override
          > > + public void close() throws Exception {
          > > + // no-op
          >
          > Controller ties to the lifecycle of the stuff that it is controlling. What
          > do mean by leave the runnable process running when closing the controller?
          > Is it related to #4 <https://github.com/apache/twill/pull/4> ?
          >
          > —
          > You are receiving this because you authored the thread.
          > Reply to this email directly, view it on GitHub
          > <https://github.com/apache/twill/pull/6/files/7b7f472f52e17d5742c88f6818ba4150a05003b5..f5716b0e5765604427225b1716d75ea590c7aaee#r76378304>,
          > or mute the thread
          > <https://github.com/notifications/unsubscribe-auth/ACU_EUN0tSXqJYX-qeGQOWRmzPOpQXYCks5qjpv6gaJpZM4JtyZN>
          > .
          >

          Show
          githubbot ASF GitHub Bot added a comment - Github user anwar6953 commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76378540 — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java — @@ -76,6 +76,11 @@ public String toString() { return new ProcessController<R>() { @Override + public void close() throws Exception { + // no-op — End diff – I just meant, now both close and cancel will kill the runnable? On Aug 26, 2016 12:55 AM, "Terence Yim" <notifications@github.com> wrote: > In twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ > RunnableProcessLauncher.java > < https://github.com/apache/twill/pull/6#discussion_r76378304 >: > > > @@ -76,6 +76,11 @@ public String toString() { > > > > return new ProcessController<R>() { > > @Override > > + public void close() throws Exception { > > + // no-op > > Controller ties to the lifecycle of the stuff that it is controlling. What > do mean by leave the runnable process running when closing the controller? > Is it related to #4 < https://github.com/apache/twill/pull/4 > ? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < https://github.com/apache/twill/pull/6/files/7b7f472f52e17d5742c88f6818ba4150a05003b5..f5716b0e5765604427225b1716d75ea590c7aaee#r76378304 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ACU_EUN0tSXqJYX-qeGQOWRmzPOpQXYCks5qjpv6gaJpZM4JtyZN > > . >
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76430188

          — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java —
          @@ -76,6 +76,11 @@ public String toString() {

          return new ProcessController<R>() {
          @Override
          + public void close() throws Exception {
          + // no-op
          — End diff –

          No. Close should just release resources

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76430188 — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java — @@ -76,6 +76,11 @@ public String toString() { return new ProcessController<R>() { @Override + public void close() throws Exception { + // no-op — End diff – No. Close should just release resources
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76455964

          — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java —
          @@ -76,6 +76,11 @@ public String toString() {

          return new ProcessController<R>() {
          @Override
          + public void close() throws Exception {
          + // no-op
          — End diff –

          So then the close method will be a no-op in this case.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anwar6953 commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76455964 — Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunnableProcessLauncher.java — @@ -76,6 +76,11 @@ public String toString() { return new ProcessController<R>() { @Override + public void close() throws Exception { + // no-op — End diff – So then the close method will be a no-op in this case.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76685365

          — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java —
          @@ -62,18 +62,26 @@
          public final class Hadoop20YarnAppClient extends AbstractIdleService implements YarnAppClient {

          private static final Logger LOG = LoggerFactory.getLogger(Hadoop20YarnAppClient.class);

          • private final YarnClient yarnClient;
            + private final Configuration configuration;
            private String user;

          public Hadoop20YarnAppClient(Configuration configuration)

          { - this.yarnClient = new YarnClientImpl(); - yarnClient.init(configuration); + this.configuration = configuration; this.user = System.getProperty("user.name"); }

          + // Creates and starts a yarn client
          + private YarnClient createYarnClient()

          { + YarnClient yarnClient = new YarnClientImpl(); + yarnClient.init(configuration); + yarnClient.start(); + return yarnClient; + }

          +
          @Override
          public ProcessLauncher<ApplicationMasterInfo> createLauncher(TwillSpecification twillSpec,
          @Nullable String schedulerQueue) throws Exception {
          + final YarnClient yarnClient = createYarnClient();
          — End diff –

          Why this one will get closed?

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76685365 — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java — @@ -62,18 +62,26 @@ public final class Hadoop20YarnAppClient extends AbstractIdleService implements YarnAppClient { private static final Logger LOG = LoggerFactory.getLogger(Hadoop20YarnAppClient.class); private final YarnClient yarnClient; + private final Configuration configuration; private String user; public Hadoop20YarnAppClient(Configuration configuration) { - this.yarnClient = new YarnClientImpl(); - yarnClient.init(configuration); + this.configuration = configuration; this.user = System.getProperty("user.name"); } + // Creates and starts a yarn client + private YarnClient createYarnClient() { + YarnClient yarnClient = new YarnClientImpl(); + yarnClient.init(configuration); + yarnClient.start(); + return yarnClient; + } + @Override public ProcessLauncher<ApplicationMasterInfo> createLauncher(TwillSpecification twillSpec, @Nullable String schedulerQueue) throws Exception { + final YarnClient yarnClient = createYarnClient(); — End diff – Why this one will get closed?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76685638

          — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java —
          @@ -62,18 +62,26 @@
          public final class Hadoop20YarnAppClient extends AbstractIdleService implements YarnAppClient {

          private static final Logger LOG = LoggerFactory.getLogger(Hadoop20YarnAppClient.class);

          • private final YarnClient yarnClient;
            + private final Configuration configuration;
            private String user;

          public Hadoop20YarnAppClient(Configuration configuration)

          { - this.yarnClient = new YarnClientImpl(); - yarnClient.init(configuration); + this.configuration = configuration; this.user = System.getProperty("user.name"); }

          + // Creates and starts a yarn client
          + private YarnClient createYarnClient()

          { + YarnClient yarnClient = new YarnClientImpl(); + yarnClient.init(configuration); + yarnClient.start(); + return yarnClient; + }

          +
          @Override
          public ProcessLauncher<ApplicationMasterInfo> createLauncher(TwillSpecification twillSpec,
          @Nullable String schedulerQueue) throws Exception {
          + final YarnClient yarnClient = createYarnClient();
          — End diff –

          Nevermind, I found it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76685638 — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java — @@ -62,18 +62,26 @@ public final class Hadoop20YarnAppClient extends AbstractIdleService implements YarnAppClient { private static final Logger LOG = LoggerFactory.getLogger(Hadoop20YarnAppClient.class); private final YarnClient yarnClient; + private final Configuration configuration; private String user; public Hadoop20YarnAppClient(Configuration configuration) { - this.yarnClient = new YarnClientImpl(); - yarnClient.init(configuration); + this.configuration = configuration; this.user = System.getProperty("user.name"); } + // Creates and starts a yarn client + private YarnClient createYarnClient() { + YarnClient yarnClient = new YarnClientImpl(); + yarnClient.init(configuration); + yarnClient.start(); + return yarnClient; + } + @Override public ProcessLauncher<ApplicationMasterInfo> createLauncher(TwillSpecification twillSpec, @Nullable String schedulerQueue) throws Exception { + final YarnClient yarnClient = createYarnClient(); — End diff – Nevermind, I found it.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/6#discussion_r76685965

          — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java —
          @@ -176,22 +184,27 @@ private void addRMToken(ContainerLaunchContext context) {

          @Override
          public ProcessController<YarnApplicationReport> createProcessController(ApplicationId appId)

          { - return new ProcessControllerImpl(yarnClient, appId); + return new ProcessControllerImpl(createYarnClient(), appId); }

          @Override
          public List<NodeReport> getNodeReports() throws Exception {

          • return this.yarnClient.getNodeReports();
            + YarnClient yarnClient = createYarnClient();
            + try { + return yarnClient.getNodeReports(); + }

            finally

            { + yarnClient.stop(); + }

            }

          @Override
          protected void startUp() throws Exception {

          • yarnClient.start();
            + // no-op. We will create and start YarnClients, on demand
              • End diff –

          Maybe we should make `YarnAppClient` not a `Service` at all. It seems like there is no need for any lifecycle management.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/6#discussion_r76685965 — Diff: twill-yarn/src/main/hadoop20/org/apache/twill/internal/yarn/Hadoop20YarnAppClient.java — @@ -176,22 +184,27 @@ private void addRMToken(ContainerLaunchContext context) { @Override public ProcessController<YarnApplicationReport> createProcessController(ApplicationId appId) { - return new ProcessControllerImpl(yarnClient, appId); + return new ProcessControllerImpl(createYarnClient(), appId); } @Override public List<NodeReport> getNodeReports() throws Exception { return this.yarnClient.getNodeReports(); + YarnClient yarnClient = createYarnClient(); + try { + return yarnClient.getNodeReports(); + } finally { + yarnClient.stop(); + } } @Override protected void startUp() throws Exception { yarnClient.start(); + // no-op. We will create and start YarnClients, on demand End diff – Maybe we should make `YarnAppClient` not a `Service` at all. It seems like there is no need for any lifecycle management.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/6

          LGTM. Can you squash the commits?

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/6 LGTM. Can you squash the commits?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/6

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

            People

            • Assignee:
              anwar1 Ali Anwar
              Reporter:
              anwar1 Ali Anwar
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development