Github user gsps1 commented on a diff in the pull request:
https://github.com/apache/incubator-twill/pull/70#discussion_r42302190
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterMain.java
---
@@ -229,4 +234,82 @@ protected void shutDown() throws Exception {
}
}
}
+
+ private static final class AppMasterTwillZKPathService extends
TwillZKPathService {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(AppMasterTwillZKPathService.class);
+ private final ZKClient zkClient;
+
+ public AppMasterTwillZKPathService(ZKClient zkClient, RunId runId) {
+ super(zkClient, runId);
+ this.zkClient = zkClient;
+ }
+
+ @Override
+ protected void shutDown() throws Exception {
+ super.shutDown();
+
+ // Deletes ZK nodes created for the application execution
+ // We don't have to worry about race condition when another instance
of the same app starts at the same time
+ // when removal is performed because we always create node with
"createParent == true", which will take care of
+ // the the parent node recreation if it is getting removed from here.
+
+ // Try to delete the /instances path. It may throws
NotEmptyException if there are other instances of the
+ // same app running, which can safely ignore and return.
+ if (!delete(Constants.INSTANCES_PATH_PREFIX)) {
+ return;
+ }
+
+ // Try to delete children under /discovery. It may fail with
NotEmptyException if there are other instances
+ // of the same app running that has discovery services running.
+ List<String> children =
zkClient.getChildren(Constants.DISCOVERY_PATH_PREFIX)
+ .get(TIMEOUT_SECONDS,
TimeUnit.SECONDS).getChildren();
+ List<OperationFuture<?>> deleteFutures = new ArrayList<>();
+ for (String child : children) {
+ String path = Constants.DISCOVERY_PATH_PREFIX + "/" + child;
+ LOG.info("Removing ZK path: {}{}", zkClient.getConnectString(),
path);
+ deleteFutures.add(zkClient.delete(path));
+ }
+ Futures.successfulAsList(deleteFutures).get(TIMEOUT_SECONDS,
TimeUnit.SECONDS);
+ for (OperationFuture<?> future : deleteFutures) {
+ try {
+ future.get();
--- End diff --
wouldn't ``Futures.successfulAsList(deleteFutures).get(TIMEOUT_SECONDS,
TimeUnit.SECONDS);`` already ensure the futures are completed?
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
