(TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed #67

chtyim · 2018-03-09T22:13:47Z

Delete the Kafka root zk node for the application if already exist
Delete the AM instance zk node if already exist
For runnables parent zk node, it is not an error if it already exist
Enhance KafkaClient publisher / consumer to deal with Kafka cluster changes
- When AM killed and restarted, the embedded Kafka will be running in different host and port

anew

I have a few comments,

anew · 2018-03-14T18:46:51Z

twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java

+      public void onFailure(final Throwable createFailure) {
+        if (!(createFailure instanceof KeeperException.NodeExistsException)) {
+          resultFuture.setException(createFailure);
+        }


do you need to return here?

That's right. Missed it.

anew · 2018-03-14T18:47:50Z

twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java

+          resultFuture.setException(createFailure);
+        }
+
+        // If the node already exists, it is due to previous run attempt that left an ephemeral node.


how can it leave an ephemeral node? Doesn't that mean there must be a zombie process holding on to that node?

Ephemeral node won't go away immediate if the process die. It will stay there till ZK session timeout, which is typically multiple seconds. In the meantime, the next AM process may already be started by YARN, hence the new AM process will see the ephemeral node.

anew · 2018-03-14T18:53:03Z

twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java

+      }
+    }, Threads.SAME_THREAD_EXECUTOR);
+
+    return resultFuture;


while this code appears correct (after addressing my comment), the three nested levels of callback make it almost impossible to read. Is there some way to unwind this?

Well, we do need that many levels of callback (create -> delete -> create) for the operation. Any suggestions on how to simplify it?

I can pull the common code between this and the ApplicationMasterMain class into a util function. But still, inside the util function, there would be three callbacks.

I was thinking it does not have to be async. Create the ZK node, get the future. If success, done. If not delete the ZK node, get the future. If failure, throw. Else try again. But maybe that would be equally complex?

anew · 2018-03-14T19:16:46Z

twill-core/src/main/java/org/apache/twill/internal/kafka/client/SimpleKafkaPublisher.java

      if (oldProducer != null) {
        oldProducer.close();
      }

-      LOG.info("Update Kafka producer broker list: {}", newBrokerList);
+      if (newBrokerList.isEmpty()) {
+        LOG.warn("Empty Kafka producer broker list, publish will fail.");


So when will this happen? If the AM dies (and its broker with it)?

anew · 2018-03-14T19:28:28Z

twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterMain.java

+        public void onFailure(final Throwable createFailure) {
+          if (!(createFailure instanceof KeeperException.NodeExistsException)) {
+            completion.setException(createFailure);
+          }


return here?

chtyim · 2018-03-14T22:12:55Z

I've addressed the comments and fixed one more issue (one deletion failure, if the node not exist, we can just go ahead and recreate the node instead of failing).

Also refactored the callback code a bit to try to make it cleaner.

anew

one minor comment, otherwise LGTM

anew · 2018-03-14T23:47:49Z

twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterMain.java

@@ -170,45 +167,7 @@ private ApplicationKafkaService(ZKClient zkClient, String kafkaZKConnect) {
    protected void startUp() throws Exception {
      // Create the ZK node for Kafka to use. If the node already exists, delete it to make sure there is
      // no left over content from previous AM attempt.
-      final SettableOperationFuture<String> completion = SettableOperationFuture.create(kafkaZKPath,
-                                                                                        Threads.SAME_THREAD_EXECUTOR);
-      LOG.info("Preparing Kafka ZK path {}{}", zkClient.getConnectString(), kafkaZKPath);


maybe keep the log message?

oh yeah, accidentally removed.

anew · 2018-03-14T23:51:01Z

twill-zookeeper/src/main/java/org/apache/twill/zookeeper/ZKOperations.java

+                                 Iterable<ACL> acls, FutureCallback<String> callback) {
+    Futures.addCallback(zkClient.create(path, data, createMode, createParent, acls),
+                        callback, Threads.SAME_THREAD_EXECUTOR);
+  }


yes, easier to understand and extracted into separate methods. Like it much better now

… first attempt failed - Delete the Kafka root zk node for the application if already exist - Delete the AM instance zk node if already exist - For runnables parent zk node, it is not an error if it already exist - Enhance KafkaClient publisher / consumer to deal with Kafka cluster changes - When AM killed and restarted, the embedded Kafka will be running in different host and port

… first attempt failed - Delete the Kafka root zk node for the application if already exist - Delete the AM instance zk node if already exist - For runnables parent zk node, it is not an error if it already exist - Enhance KafkaClient publisher / consumer to deal with Kafka cluster changes - When AM killed and restarted, the embedded Kafka will be running in different host and port This closes #67 on Github. Signed-off-by: Terence Yim <chtyim@apache.org>

chtyim force-pushed the feature/TWILL-61 branch 2 times, most recently from d596d4d to 14c38d4 Compare March 12, 2018 23:10

anew reviewed Mar 14, 2018

View reviewed changes

chtyim force-pushed the feature/TWILL-61 branch from 07c418c to 27db864 Compare March 15, 2018 00:01

asfgit closed this in 107dc1e Mar 15, 2018

chtyim deleted the feature/TWILL-61 branch March 19, 2018 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed #67

(TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed #67

chtyim commented Mar 9, 2018

anew left a comment

anew Mar 14, 2018

chtyim Mar 14, 2018

anew Mar 14, 2018

chtyim Mar 14, 2018

anew Mar 14, 2018

anew Mar 14, 2018

chtyim Mar 14, 2018

chtyim Mar 14, 2018

anew Mar 14, 2018

anew Mar 14, 2018

chtyim Mar 14, 2018

anew Mar 14, 2018

chtyim commented Mar 14, 2018

anew left a comment

anew Mar 14, 2018

chtyim Mar 14, 2018

anew Mar 14, 2018

(TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed #67

(TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed #67

Conversation

chtyim commented Mar 9, 2018

anew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chtyim commented Mar 14, 2018

anew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment