[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120944#comment-16120944
 ] 

ASF GitHub Bot commented on TWILL-240:
--

Github user asfgit closed the pull request at:

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


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120825#comment-16120825
 ] 

ASF GitHub Bot commented on TWILL-240:
--

Github user maochf commented on the issue:

https://github.com/apache/twill/pull/58
  
squashed


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120748#comment-16120748
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132322898
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -239,14 +359,8 @@ protected void doStop() throws Exception {
 LOG.info("Stop application master with spec: {}",
  
TwillRuntimeSpecificationAdapter.create().toJson(twillRuntimeSpec));
 
-if (eventHandler != null) {
-  try {
-// call event handler destroy. If there is error, only log and not 
affected stop sequence.
-eventHandler.destroy();
-  } catch (Throwable t) {
-LOG.warn("Exception when calling {}.destroy()", 
eventHandler.getClass().getName(), t);
-  }
-}
+// call event handler destroy
+eventHandler.destroy();
--- End diff --

I think so.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120746#comment-16120746
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132322804
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -239,14 +359,8 @@ protected void doStop() throws Exception {
 LOG.info("Stop application master with spec: {}",
  
TwillRuntimeSpecificationAdapter.create().toJson(twillRuntimeSpec));
 
-if (eventHandler != null) {
-  try {
-// call event handler destroy. If there is error, only log and not 
affected stop sequence.
-eventHandler.destroy();
-  } catch (Throwable t) {
-LOG.warn("Exception when calling {}.destroy()", 
eventHandler.getClass().getName(), t);
-  }
-}
+// call event handler destroy
+eventHandler.destroy();
--- End diff --

call it at the end of `doStop() `?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120745#comment-16120745
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132322316
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -239,14 +359,8 @@ protected void doStop() throws Exception {
 LOG.info("Stop application master with spec: {}",
  
TwillRuntimeSpecificationAdapter.create().toJson(twillRuntimeSpec));
 
-if (eventHandler != null) {
-  try {
-// call event handler destroy. If there is error, only log and not 
affected stop sequence.
-eventHandler.destroy();
-  } catch (Throwable t) {
-LOG.warn("Exception when calling {}.destroy()", 
eventHandler.getClass().getName(), t);
-  }
-}
+// call event handler destroy
+eventHandler.destroy();
--- End diff --

Shouldn't call `destroy()` here anymore. It should be called after call 
other calls to the event handler, as it is acted as a cleanup call.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120743#comment-16120743
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132322087
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -181,19 +190,130 @@ public Reader getInput() throws IOException {
   }
 
   @SuppressWarnings("unchecked")
-  @Nullable
   private EventHandler createEventHandler(TwillSpecification twillSpec) 
throws ClassNotFoundException {
 // Should be able to load by this class ClassLoader, as they packaged 
in the same jar.
 EventHandlerSpecification handlerSpec = twillSpec.getEventHandler();
 if (handlerSpec == null) {
-  return null;
+  // if no handler is specified, return an EventHandler with no-op
+  return new EventHandler() {};
 }
 
 Class handlerClass = 
getClass().getClassLoader().loadClass(handlerSpec.getClassName());
 
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
 "Class {} does not implements {}",
 handlerClass, 
EventHandler.class.getName());
-return Instances.newInstance((Class) 
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+// wrap all calls to the delegate EventHandler methods except 
initialize so that all errors will be caught
+return new EventHandler() {
+  private boolean initialized;
+
+  @Override
+  public void initialize(EventHandlerContext context) {
+delegate.initialize(context);
+initialized = true;
--- End diff --

Since any exception thrown from `initialize` will terminate the app, I 
think we don't need this, right?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120614#comment-16120614
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132298585
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -193,7 +200,91 @@ private EventHandler 
createEventHandler(TwillSpecification twillSpec) throws Cla
 
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
 "Class {} does not implements {}",
 handlerClass, 
EventHandler.class.getName());
-return Instances.newInstance((Class) 
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+  // if no handler is specified, return an EventHandler with no-op
+  return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
--- End diff --

originally, if `EventHandler#initialize` fails, the app will fail to start. 
Do we want to change this behavior?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120612#comment-16120612
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132297954
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -401,6 +409,24 @@ void stopAll() {
 // When we acquire this lock, all stopped runnables should have been 
cleaned up by handleCompleted() method
 containerLock.lock();
 try {
+  for (Map.Entry> entry 
: containers.rowMap().entrySet()) {
+String runnableName = entry.getKey();
+Collection containerInfos = 
containerStats.get(runnableName);
+for (Map.Entry 
containerControllerEntry : entry.getValue().entrySet()) {
+  boolean containerExist = false;
+  for (ContainerInfo containerInfo : containerInfos) {
+if 
(containerInfo.getId().equals(containerControllerEntry.getKey())) {
+  containerExist = true;
+  break;
+}
+  }
+  // Only call eventHandler.containerStopped if container is not 
removed by handleCompleted
+  if (containerExist) {
+eventHandler.containerStopped(runnableName, 
containerControllerEntry.getValue().getInstanceId(),
--- End diff --

you can move this inside the `for` loop and no need to use `containerExist`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120609#comment-16120609
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132297305
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -193,7 +200,91 @@ private EventHandler 
createEventHandler(TwillSpecification twillSpec) throws Cla
 
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
 "Class {} does not implements {}",
 handlerClass, 
EventHandler.class.getName());
-return Instances.newInstance((Class) 
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+  // if no handler is specified, return an EventHandler with no-op
+  return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
--- End diff --

You'll need to wrap the `initialize` as well. Also, I'd suggest if 
`initialize` throw exception, you'll just ignore all calls to other lifecycle 
methods to avoid excessive logging.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120603#comment-16120603
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132296198
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -193,7 +200,91 @@ private EventHandler 
createEventHandler(TwillSpecification twillSpec) throws Cla
 
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
 "Class {} does not implements {}",
 handlerClass, 
EventHandler.class.getName());
-return Instances.newInstance((Class) 
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
--- End diff --

`Instances.newInstance` never return `null`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16120602#comment-16120602
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132295847
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -181,7 +189,6 @@ public Reader getInput() throws IOException {
   }
 
   @SuppressWarnings("unchecked")
-  @Nullable
   private EventHandler createEventHandler(TwillSpecification twillSpec) 
throws ClassNotFoundException {
 // Should be able to load by this class ClassLoader, as they packaged 
in the same jar.
 EventHandlerSpecification handlerSpec = twillSpec.getEventHandler();
--- End diff --

if `handlerSpec` is `null`, you still want to return a no-op handler, right?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119110#comment-16119110
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132048851
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -267,6 +280,14 @@ void stopByIdAndWait(String runnableName, int 
instanceId) {
 
 resourceReport.removeRunnableResources(runnableName, containerId);
 containerChange.signalAll();
+if (eventHandler != null) {
+  Integer exitStatus = containerExitStatus.get(containerId);
+  if (exitStatus == null) {
--- End diff --

`controller.stopAndWait();` at line 269 blocks until `handleCompleted` 
method runs


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119073#comment-16119073
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132038521
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -267,6 +280,14 @@ void stopByIdAndWait(String runnableName, int 
instanceId) {
 
 resourceReport.removeRunnableResources(runnableName, containerId);
 containerChange.signalAll();
+if (eventHandler != null) {
+  Integer exitStatus = containerExitStatus.get(containerId);
--- End diff --

Should remove it from the exit status map, otherwise the map can keep 
growing in size.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119072#comment-16119072
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132037568
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -221,7 +229,8 @@ protected void doStart() throws Exception {
 
 // initialize the event handler, if it fails, it will fail the 
application.
 if (eventHandler != null) {
-  eventHandler.initialize(new 
BasicEventHandlerContext(twillSpec.getEventHandler()));
+  eventHandler.initialize(new 
BasicEventHandlerContext(twillRuntimeSpec, twillSpec.getEventHandler()));
--- End diff --

The `twillRuntimeSpec` already contains `twillSpec`. Seems unnecessary to 
pass in `twillSpec.getEventHandler()` as a separate parameter.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119074#comment-16119074
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132040093
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -267,6 +280,14 @@ void stopByIdAndWait(String runnableName, int 
instanceId) {
 
 resourceReport.removeRunnableResources(runnableName, containerId);
 containerChange.signalAll();
+if (eventHandler != null) {
+  Integer exitStatus = containerExitStatus.get(containerId);
+  if (exitStatus == null) {
--- End diff --

So this is for handling the case when `handleCompleted` was not called 
(e.g. the runnable container is not stopping after receiving the "stop" 
command), right? If that's the case, isn't that the exit status is always not 
there?

Also, do we need to do the similar logic to call event handler in the 
`stopAll` method?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119075#comment-16119075
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132038164
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -173,6 +183,9 @@ void start(String runnableName, ContainerInfo 
containerInfo, TwillContainerLaunc
 startSequence.addLast(runnableName);
   }
   containerChange.signalAll();
+  if (eventHandler != null) {
+eventHandler.containerLaunched(runnableName, instanceId, 
containerInfo.getId());
--- End diff --

How do we handle exception raised from event handler methods?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119022#comment-16119022
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132029465
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.yarn;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
+  private static final Logger LOG = 
LoggerFactory.getLogger(EventHandlerTest.class);
+
+  @ClassRule
+  public static final TemporaryFolder TMP_FOLDER = new TemporaryFolder();
+  public static final String STARTED_FILE = "started_file";
+  public static final String RUN_FILE = "run_file";
+  public static final String CONTAINER_LAUNCHED_FOLDER = "launched_folder";
+  public static final String CONTAINER_STOPPED_FOLDER = "stopped_folder";
+  public static final String COMPLETED_FILE = "completed_file";
+  public static final String KILLED_FILE = "killed_file";
+  public static final String ABORTED_FILE = "aborted_file";
+
+  @Test
+  public void testComplete() throws InterruptedException, 
ExecutionException, TimeoutException, IOException {
+// Create a parent folder to be written by EventHandler
+File parentFolder = TMP_FOLDER.newFolder();
+parentFolder.setWritable(true, false);
+TwillController controller = getTwillRunner().prepare(new 
CompleteApplication(parentFolder.getAbsolutePath()))
+  .addLogHandler(new PrinterLogHandler(new PrintWriter(System.out, 
true)))
+  .start();
+
+// Wait for the app to complete within 120 seconds.
+try {
+  controller.awaitTerminated(120, TimeUnit.SECONDS);
+  Set handlerFiles = new 
HashSet<>(Arrays.asList(parentFolder.list()));
+  Assert.assertEquals(5, handlerFiles.size());
+  // EventHandler#started() method should be called to create a file
+  Assert.assertTrue(handlerFiles.contains(STARTED_FILE));
+  // CompleteRunnable#run() method should be called to create a file 
after EventHandler#started() method is called
+  Assert.assertTrue(handlerFiles.contains(RUN_FILE));
+  // EventHandler#containerLaunched(String, int, String) method should 
be called to create a folder
+  Assert.assertTrue(handlerFiles.contains(CONTAINER_LAUNCHED_FOLDER));
+  // EventHandler#containerStopped(String, int, String, int) method 
should be called to create a folder
+  Assert.assertTrue(handlerFiles.contains(CONTAINER_STOPPED_FOLDER));
+  // Assert that containerLaunched and containerStopped are called for 
the 

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16119021#comment-16119021
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r132029365
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.yarn;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
--- End diff --

Rename the test class to `EventHandlerTestRun` and add it to 
`YarnTestSuite`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117272#comment-16117272
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131761743
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
  */
 public interface EventHandlerContext {
 
+  String getTwillAppName();
--- End diff --

Should be called `getApplicationName()`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117276#comment-16117276
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131761787
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
  */
 public interface EventHandlerContext {
 
+  String getTwillAppName();
+
+  String getTwillAppRunId();
--- End diff --

Just call it `getRunId()`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117277#comment-16117277
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131762365
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
  */
 public interface EventHandlerContext {
 
+  String getTwillAppName();
+
+  String getTwillAppRunId();
--- End diff --

The run id should be of type `RunId`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117271#comment-16117271
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131761843
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
  */
 public interface EventHandlerContext {
 
+  String getTwillAppName();
--- End diff --

Also please add javadoc.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117274#comment-16117274
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131762894
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction 
launchTimeout(Iterable timeoutEvents) {
 return TimeoutAction.recheck(10, TimeUnit.SECONDS);
   }
 }
+
+@Override
+public void aborted() {
+  try {
+new 
File(context.getSpecification().getConfigs().get("parentFolderPath") + 
File.separator
--- End diff --

Also, we are only testing abort? How about other lifecycle methods?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117273#comment-16117273
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131762189
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -291,6 +300,24 @@ public void run() {
 // Since all the runnables are now stopped, it is okay to stop the 
poller.
 stopPoller.shutdownNow();
 cleanupDir();
+if (eventHandler != null) {
+  if (stopStatus == null) {
+// if finalStatus is not set, the application must be stopped by a 
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
+return;
+  }
+  switch (stopStatus) {
+case COMPLETED:
+  eventHandler.completed();
+  break;
+case ABORTED:
+eventHandler.aborted();
--- End diff --

Misalignment.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16117275#comment-16117275
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131762593
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction 
launchTimeout(Iterable timeoutEvents) {
 return TimeoutAction.recheck(10, TimeUnit.SECONDS);
   }
 }
+
+@Override
+public void aborted() {
+  try {
+new 
File(context.getSpecification().getConfigs().get("parentFolderPath") + 
File.separator
--- End diff --

When constructing file, usually not to use `File.separator`, but instead 
use the constructor `File(File parent, String path)`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
> Fix For: 0.12.0
>
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113320#comment-16113320
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131234842
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/EventHandler.java ---
@@ -124,6 +124,75 @@ public void initialize(EventHandlerContext context) {
   }
 
   /**
+   * Invoked by the application when it starts.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void started(String twillAppName, RunId runId) {
--- End diff --

the `EventHandlerContext` just contains a `EventHandlerSpecification`, and 
it doesn't seem to guarantee app name and runid are there?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113254#comment-16113254
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131225251
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -79,12 +83,22 @@ public void initialize(EventHandlerContext context) {
 
 @Override
 public TimeoutAction launchTimeout(Iterable 
timeoutEvents) {
+  for (TimeoutEvent event : timeoutEvents) {
+LOG.info("Requested {} containers for runnable {}, only got {} 
after {} ms.",
--- End diff --

i was using it to confirm the change. Will remove


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113163#comment-16113163
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131210382
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -291,6 +301,23 @@ public void run() {
 // Since all the runnables are now stopped, it is okay to stop the 
poller.
 stopPoller.shutdownNow();
 cleanupDir();
+if (eventHandler != null) {
+  if (finalStatus == null) {
+// if finalStatus is not set, the application must be stopped by a 
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
+  }
+  switch (finalStatus) {
--- End diff --

This may have NPE. Should have a `else`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113161#comment-16113161
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131210237
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -291,6 +301,23 @@ public void run() {
 // Since all the runnables are now stopped, it is okay to stop the 
poller.
 stopPoller.shutdownNow();
 cleanupDir();
+if (eventHandler != null) {
+  if (finalStatus == null) {
+// if finalStatus is not set, the application must be stopped by a 
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
+  }
+  switch (finalStatus) {
+case COMPLETED:
+  eventHandler.completed(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
+  break;
+case ABORTED:
+eventHandler.aborted(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
+  break;
+default:
+  // should never reach here
+  LOG.error("Unsupported FinalStatus '%s'", finalStatus.name());
--- End diff --

That's not the right syntax. slf4j logger uses `{}`. Should be 
`LOG.error("Unsupported status {}", finalStatus)`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113159#comment-16113159
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131208560
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/EventHandler.java ---
@@ -124,6 +124,74 @@ public void initialize(EventHandlerContext context) {
   }
 
   /**
+   * Invoked by the application when it starts.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void started(String twillAppName, RunId runId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when new container is launched for {@link 
TwillRunnable}.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   * @param runnableName name of the runnable to be run in the new 
container
+   * @param instanceId the instance ID of the runnable instance to be run 
in the new container
+   * @param containerId the ID of the newly launched container
+   */
+  public void containerLaunched(String twillAppName, RunId runId, String 
runnableName,
+int instanceId, String containerId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when a container is stopped.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   * @param runnableName name of the runnable in the stopped container
+   * @param instanceId the instance ID of the runnable instance run in the 
stopped container
+   * @param containerId the ID of the stopped container
+   */
+  public void containerStopped(String twillAppName, RunId runId, String 
runnableName,
+   int instanceId, String containerId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when all containers complete.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void completed(String twillAppName, RunId runId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when stop command is received to kill the 
current application.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void killed(String twillAppName, RunId runId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when the application is aborted because of 
timeout.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void aborted(String twillAppName, RunId runId) {
+// No-op
+  }
+
+  /**
* Invoked by the application when shutting down.
*/
   public void destroy() {
--- End diff --

continue, with the recheck time to some hardcode constant (say 60 seconds).


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113165#comment-16113165
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131212239
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -79,12 +83,22 @@ public void initialize(EventHandlerContext context) {
 
 @Override
 public TimeoutAction launchTimeout(Iterable 
timeoutEvents) {
+  for (TimeoutEvent event : timeoutEvents) {
+LOG.info("Requested {} containers for runnable {}, only got {} 
after {} ms.",
+ event.getExpectedInstances(), event.getRunnableName(),
+ event.getActualInstances(), System.currentTimeMillis() - 
event.getRequestTime());
+  }
   if (abort) {
 return TimeoutAction.abort();
   } else {
 return TimeoutAction.recheck(10, TimeUnit.SECONDS);
   }
 }
+
+@Override
+public void aborted(String twillAppName, RunId runId) {
+  LOG.info(String.format("Aborted %s with runId %s", twillAppName, 
runId.getId()));
--- End diff --

Besides logging, can you add/modify the test to actually validate the new 
lifecycle methods in the EventHandler?


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113164#comment-16113164
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r131209163
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -135,6 +135,15 @@
   private final Map> environments;
   private final TwillRuntimeSpecification twillRuntimeSpec;
 
+  /**
+   * Final status of this service when it stops.
+   */
+  public enum FinalStatus {
--- End diff --

Sounds more like `StopStatus` or `CompletionStatus`. Also it should be 
`private` since it is only used in this class.

Also, please move it before all fields declaration instead of having it in 
between fields.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16107647#comment-16107647
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r130406951
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/EventHandler.java ---
@@ -124,6 +124,74 @@ public void initialize(EventHandlerContext context) {
   }
 
   /**
+   * Invoked by the application when it starts.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   */
+  public void started(String twillAppName, RunId runId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when new container is launched for {@link 
TwillRunnable}.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   * @param runnableName name of the runnable to be run in the new 
container
+   * @param instanceId the instance ID of the runnable instance to be run 
in the new container
+   * @param containerId the ID of the newly launched container
+   */
+  public void containerLaunched(String twillAppName, RunId runId, String 
runnableName,
+int instanceId, String containerId) {
+// No-op
+  }
+
+  /**
+   * Invoked by the application when a container is stopped.
+   *
+   * @param twillAppName name of the current application
+   * @param runId run ID of current application run
+   * @param runnableName name of the runnable in the stopped container
+   * @param instanceId the instance ID of the runnable instance run in the 
stopped container
+   * @param containerId the ID of the stopped container
+   */
+  public void containerStopped(String twillAppName, RunId runId, String 
runnableName,
--- End diff --

Should provide the exit code as well.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16107645#comment-16107645
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r130407739
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -205,8 +208,9 @@ private RunningContainers 
createRunningContainers(ContainerId appMasterContainer
   Integer.parseInt(System.getenv(EnvKeys.YARN_CONTAINER_MEMORY_MB)),
   appMasterHost, null);
 String appId = 
appMasterContainerId.getApplicationAttemptId().getApplicationId().toString();
-return new RunningContainers(appId, appMasterResources, zkClient, 
applicationLocation,
-  twillSpec.getRunnables(), twillRuntimeSpec.getMaxRetries());
+return new RunningContainers(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId(),
--- End diff --

Better just pass the `twillRuntimeSpec` to the `RunningContainers` instead 
of getting individual entries one by one. It can replace the 
`twillRuntimeSpec.getXXX` and `twillSpec.getRunnables`.


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16102010#comment-16102010
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r129647384
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -222,6 +226,7 @@ protected void doStart() throws Exception {
 // initialize the event handler, if it fails, it will fail the 
application.
 if (eventHandler != null) {
   eventHandler.initialize(new 
BasicEventHandlerContext(twillSpec.getEventHandler()));
+  eventHandler.started(twillRuntimeSpec.getTwillAppName(), 
twillRuntimeSpec.getTwillAppRunId());
--- End diff --

If initialization fails, then the whole app will fail. `TwillController` 
should be able to catch this


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16101983#comment-16101983
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r129643121
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -267,6 +281,9 @@ void stopByIdAndWait(String runnableName, int 
instanceId) {
 
 resourceReport.removeRunnableResources(runnableName, containerId);
 containerChange.signalAll();
+if (eventHandler != null) {
+  eventHandler.containerLaunched(twillAppName, runId, 
runnableName, instanceId, containerId);
--- End diff --

Typo - should be `containerStopped`


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16101982#comment-16101982
 ] 

ASF GitHub Bot commented on TWILL-240:
--

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

https://github.com/apache/twill/pull/58#discussion_r129643180
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -103,6 +105,8 @@ public Integer apply(BitSet input) {
   private final Table containers;
 
   // Map from runnableName to a BitSet, with the  bit turned 
on for having an instance running.
+  private final String twillAppName;
--- End diff --

The comment above this line should be moved down to above the 
`runnableInstances` variable


> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TWILL-240) Improve EventHandler to handle more application lifecycle events

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TWILL-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16101281#comment-16101281
 ] 

ASF GitHub Bot commented on TWILL-240:
--

GitHub user maochf opened a pull request:

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

[TWILL-240] EventHandler Improvement

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

Add started, containerLaunched, containerStopped, completed, killed, 
aborted to EventHandler

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

$ git pull https://github.com/maochf/twill feature/event-handler-improvement

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

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


commit 76eeca04586712123c44caf5788c17e0968a
Author: Chengfeng 
Date:   2017-07-25T19:53:26Z

[TWILL-240] Add started, containerLaunched, containerStopped, completed, 
killed, aborted to EventHandler




> Improve EventHandler to handle more application lifecycle events
> 
>
> Key: TWILL-240
> URL: https://issues.apache.org/jira/browse/TWILL-240
> Project: Apache Twill
>  Issue Type: New Feature
>Reporter: Chengfeng Mao
>Assignee: Chengfeng Mao
>
> Application Master should be able to run application specific code when 
> certain lifecycle events happen by calling methods from EventHandler. For 
> instance, when the app first starts, completes, aborts and etc.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)