[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/6 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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
createProcessController(ApplicationId appId) {
-return new ProcessControllerImpl(yarnClient, appId);
+return new ProcessControllerImpl(createYarnClient(), appId);
}
@Override
public List 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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
createLauncher(TwillSpecification twillSpec,
@Nullable
String schedulerQueue) throws Exception {
+final YarnClient yarnClient = createYarnClient();
--- End diff --
Nevermind, I found it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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
createLauncher(TwillSpecification twillSpec,
@Nullable
String schedulerQueue) throws Exception {
+final YarnClient yarnClient = createYarnClient();
--- End diff --
Why this one will get closed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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() {
@Override
+ public void close() throws Exception {
+// no-op
--- End diff --
So then the close method will be a no-op in this case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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() {
@Override
+ public void close() throws Exception {
+// no-op
--- End diff --
No. Close should just release resources
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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() {
@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" wrote:
> In twill-yarn/src/main/java/org/apache/twill/internal/appmaster/
> RunnableProcessLauncher.java
> :
>
> > @@ -76,6 +76,11 @@ public String toString() {
> >
> > return new ProcessController() {
> >@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 ?
>
> â
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
>
,
> or mute the thread
>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #6: TWILL-175 Avoid caching YarnClient
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() {
@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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
