[jira] [Reopened] (TWILL-61) Second launch attempt of AM always failed

2017-03-22 Thread Andreas Neumann (JIRA)

 [ 
https://issues.apache.org/jira/browse/TWILL-61?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Andreas Neumann reopened TWILL-61:
--

Seems that this is not fixed yet. We are seeing this in 0.8:
{noformat}
org.apache.zookeeper.KeeperException$NodeExistsException: KeeperErrorCode = 
NodeExists for 
/SERVICENAME.REDACTED/144ef027-672b-4a87-b88e-6b7abcf35c2c/runnables

at 
com.google.common.util.concurrent.AbstractFuture$Sync.getValue(AbstractFuture.java:294)

at 
com.google.common.util.concurrent.AbstractFuture$Sync.get(AbstractFuture.java:281)

at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:116)

at 
org.apache.twill.internal.appmaster.ApplicationMasterService.doStart(ApplicationMasterService.java:240)

at 
org.apache.twill.internal.AbstractTwillService.startUp(AbstractTwillService.java:171)

at 
com.google.common.util.concurrent.AbstractExecutionThreadService$1$1.run(AbstractExecutionThreadService.java:47)
{noformat}

> Second launch attempt of AM always failed
> -
>
> Key: TWILL-61
> URL: https://issues.apache.org/jira/browse/TWILL-61
> Project: Apache Twill
>  Issue Type: Bug
>  Components: yarn
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.5.0-incubating
>
>
> YARN would make multiple attempts to launch an application. Currently second 
> or above attempts would always fail due to creation of /runId/state node in 
> ZK fail (node exists) because runId is generated on client side and doesn't 
> change between attempts.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)


[GitHub] twill issue #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread chtyim
Github user chtyim commented on the issue:

https://github.com/apache/twill/pull/40
  
@hsaputra Thanks for the review. Please see my replies.


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread chtyim
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107347828
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
   YarnTwillController(String appName, RunId runId, ZKClient zkClient,
   final ApplicationMasterLiveNodeData amLiveNodeData, 
final YarnAppClient yarnAppClient) {
-super(runId, zkClient, Collections.emptyList());
+super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != 
null, Collections.emptyList());
--- End diff --

Or are you suggesting adding a new method `boolean isLogCollectionEnabled` 
to the `ApplicationMasterLiveNode` class?


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread chtyim
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107347590
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
 ---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
   public Map getMaxRetries() {
 return maxRetries;
   }
+
+  /**
+   * Returns the ZK connection string for the Kafka used for log 
collections,
+   * or {@code null} if log collection is disabled.
+   */
+  @Nullable
+  public String getKafkaZKConnect() {
+if (!isLogCollectionEnabled()) {
--- End diff --

What do mean by side effect check? The contract of this method is to return 
`null` if log collection is disabled.


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread hsaputra
Github user hsaputra commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107347185
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
   YarnTwillController(String appName, RunId runId, ZKClient zkClient,
   final ApplicationMasterLiveNodeData amLiveNodeData, 
final YarnAppClient yarnAppClient) {
-super(runId, zkClient, Collections.emptyList());
+super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != 
null, Collections.emptyList());
--- End diff --

Can the constructor of `YarnTwillController` take new input of 
`logCollectionEnabled` ? I am seeing different ways to check whether to disable 
log or not


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread hsaputra
Github user hsaputra commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107346167
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java 
---
@@ -71,21 +71,37 @@
   private static final Logger LOG = 
LoggerFactory.getLogger(AbstractTwillController.class);
   private static final Gson GSON = new Gson();
 
+  private final String appName;
+  private final RunId runId;
   private final Queue logHandlers;
   private final KafkaClientService kafkaClient;
   private ZKDiscoveryService discoveryServiceClient;
   private Cancellable logCancellable;
 
-  public AbstractTwillController(RunId runId, ZKClient zkClient, 
Iterable logHandlers) {
+  public AbstractTwillController(String appName, RunId runId, ZKClient 
zkClient, boolean logCollectionEnabled,
+ Iterable logHandlers) {
 super(runId, zkClient);
+this.appName = appName;
+this.runId = runId;
 this.logHandlers = new ConcurrentLinkedQueue<>();
-this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
-Iterables.addAll(this.logHandlers, logHandlers);
+
+// When addressing TWILL-147, need to check if the given ZKClient is
+// actually used by the Kafka used for log collection
+if (logCollectionEnabled) {
+  this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
+  Iterables.addAll(this.logHandlers, logHandlers);
+} else {
+  this.kafkaClient = null;
+  if (!Iterables.isEmpty(logHandlers)) {
+LOG.warn("Log collection is disabled for application {} with runId 
{}. " +
+   "Adding log handler won't get any logs.", appName, 
runId);
+  }
+}
   }
 
   @Override
   protected synchronized void doStartUp() {
-if (!logHandlers.isEmpty()) {
+if (kafkaClient != null && !logHandlers.isEmpty()) {
--- End diff --

But `this.logHandlers` will always be empty if `if (kafkaClient == null)` 
is true? So could we just rely on check on kafkaClient as null ?


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread chtyim
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107344859
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java 
---
@@ -71,21 +71,37 @@
   private static final Logger LOG = 
LoggerFactory.getLogger(AbstractTwillController.class);
   private static final Gson GSON = new Gson();
 
+  private final String appName;
+  private final RunId runId;
   private final Queue logHandlers;
   private final KafkaClientService kafkaClient;
   private ZKDiscoveryService discoveryServiceClient;
   private Cancellable logCancellable;
 
-  public AbstractTwillController(RunId runId, ZKClient zkClient, 
Iterable logHandlers) {
+  public AbstractTwillController(String appName, RunId runId, ZKClient 
zkClient, boolean logCollectionEnabled,
+ Iterable logHandlers) {
 super(runId, zkClient);
+this.appName = appName;
+this.runId = runId;
 this.logHandlers = new ConcurrentLinkedQueue<>();
-this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
-Iterables.addAll(this.logHandlers, logHandlers);
+
+// When addressing TWILL-147, need to check if the given ZKClient is
+// actually used by the Kafka used for log collection
+if (logCollectionEnabled) {
+  this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
+  Iterables.addAll(this.logHandlers, logHandlers);
+} else {
+  this.kafkaClient = null;
+  if (!Iterables.isEmpty(logHandlers)) {
+LOG.warn("Log collection is disabled for application {} with runId 
{}. " +
+   "Adding log handler won't get any logs.", appName, 
runId);
+  }
+}
   }
 
   @Override
   protected synchronized void doStartUp() {
-if (!logHandlers.isEmpty()) {
+if (kafkaClient != null && !logHandlers.isEmpty()) {
--- End diff --

`logCollectionEnabled` is not a field. I used `kafkaClient == null` to 
indicate disabling of log collection.
Also, the `logHandlers` collection can be empty during startup of the 
controller, even log collection is enabled, hence the check.


---
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 #40: (TWILL-122) Allow disabling log collection

2017-03-22 Thread hsaputra
Github user hsaputra commented on a diff in the pull request:

https://github.com/apache/twill/pull/40#discussion_r107344378
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java 
---
@@ -71,21 +71,37 @@
   private static final Logger LOG = 
LoggerFactory.getLogger(AbstractTwillController.class);
   private static final Gson GSON = new Gson();
 
+  private final String appName;
+  private final RunId runId;
   private final Queue logHandlers;
   private final KafkaClientService kafkaClient;
   private ZKDiscoveryService discoveryServiceClient;
   private Cancellable logCancellable;
 
-  public AbstractTwillController(RunId runId, ZKClient zkClient, 
Iterable logHandlers) {
+  public AbstractTwillController(String appName, RunId runId, ZKClient 
zkClient, boolean logCollectionEnabled,
+ Iterable logHandlers) {
 super(runId, zkClient);
+this.appName = appName;
+this.runId = runId;
 this.logHandlers = new ConcurrentLinkedQueue<>();
-this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
-Iterables.addAll(this.logHandlers, logHandlers);
+
+// When addressing TWILL-147, need to check if the given ZKClient is
+// actually used by the Kafka used for log collection
+if (logCollectionEnabled) {
+  this.kafkaClient = new 
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() + 
"/kafka"));
+  Iterables.addAll(this.logHandlers, logHandlers);
+} else {
+  this.kafkaClient = null;
+  if (!Iterables.isEmpty(logHandlers)) {
+LOG.warn("Log collection is disabled for application {} with runId 
{}. " +
+   "Adding log handler won't get any logs.", appName, 
runId);
+  }
+}
   }
 
   @Override
   protected synchronized void doStartUp() {
-if (!logHandlers.isEmpty()) {
+if (kafkaClient != null && !logHandlers.isEmpty()) {
--- End diff --

Should this just check `if (logCollectionEnabled)` instead since 
technically both conditions will be met when logCollectionEnabled is true


---
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.
---