aurora git commit: Close `PathChildrenCache` before its framework.

[js...@apache.org]

Repository: aurora
Updated Branches:
  refs/heads/master 1103571df -> 311b8924d


Close `PathChildrenCache` before its framework.

Previously these lifecycles were modeled as independent when, in fact,
a `CuratorFramework`'s clients must be closed befor it is closed to
prevent errors in the clients from attempting to use a closed
`CuratorFramework`.

The proof that closing was always safe already existed in
`CuratorServiceGroupMonitorTest::testExceptionalLifecycle`, but this
safety is now documented and more explicitly tested.

Bugs closed: AURORA-1729

Reviewed at https://reviews.apache.org/r/49578/


Project: http://git-wip-us.apache.org/repos/asf/aurora/repo
Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/311b8924
Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/311b8924
Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/311b8924

Branch: refs/heads/master
Commit: 311b8924ded0c11435f08c1497fce1183eceef81
Parents: 1103571
Author: John Sirois <js...@apache.org>
Authored: Mon Jul 4 08:13:33 2016 -0600
Committer: John Sirois <jo...@gmail.com>
Committed: Mon Jul 4 08:13:33 2016 -0600

----------------------------------------------------------------------
 .../discovery/CuratorServiceDiscoveryModule.java     | 15 ++++++++++++++-
 .../discovery/CuratorServiceGroupMonitor.java        | 10 ++++++++++
 .../discovery/CuratorServiceGroupMonitorTest.java    | 10 +++++++++-
 3 files changed, 33 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/aurora/blob/311b8924/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java b/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
index 2656662..999a542 100644
--- a/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
+++ b/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceDiscoveryModule.java
@@ -155,12 +155,25 @@ class CuratorServiceDiscoveryModule extends PrivateModule {
   @Singleton
   @Exposed
   ServiceGroupMonitor provideServiceGroupMonitor(
+      ShutdownRegistry shutdownRegistry,
       CuratorFramework client,
       Codec<ServiceInstance> codec) {
 
     PathChildrenCache groupCache =
         new PathChildrenCache(client, discoveryPath, true /* cacheData */);
-    return new CuratorServiceGroupMonitor(groupCache, MEMBER_SELECTOR, codec);
+
+    // NB: Even though we do not start the serviceGroupMonitor here, the registered close shutdown
+    // action is safe since the underlying PathChildrenCache close is tolerant of an un-started
+    // state. Its also crucial so that its underlying groupCache is closed prior to its
+    // curatorFramework dependency in the case when the PathChildrenCache is in fact started (via
+    // CuratorServiceGroupMonitor::start) since a CuratorFramework should have no active clients
+    // when it is closed to avoid errors in those clients when attempting to use it.
+
+    ServiceGroupMonitor serviceGroupMonitor =
+        new CuratorServiceGroupMonitor(groupCache, MEMBER_SELECTOR, codec);
+    shutdownRegistry.addAction(groupCache::close);
+
+    return serviceGroupMonitor;
   }
 
   @Provides

http://git-wip-us.apache.org/repos/asf/aurora/blob/311b8924/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java b/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
index 9d8b7bd..0b86fb6 100644
--- a/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
+++ b/src/main/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitor.java
@@ -79,6 +79,16 @@ class CuratorServiceGroupMonitor implements ServiceGroupMonitor {
     }
   }
 
+  /**
+   * The complement of {@link #start()}; stops service group monitoring activities.
+   *
+   * NB: This operation idempotent; a close can be safely called regardless of the current state of
+   * this service group monitor and only if in a started state will action be taken; otherwise close
+   * will no-op.
+   *
+   * @throws IOException if there is a problem stopping any of the service group monitoring
+   *                     activities.
+   */
   @Override
   public void close() throws IOException {
     groupCache.close();

http://git-wip-us.apache.org/repos/asf/aurora/blob/311b8924/src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java b/src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
index 1669205..e6b57ee 100644
--- a/src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/discovery/CuratorServiceGroupMonitorTest.java
@@ -33,12 +33,20 @@ public class CuratorServiceGroupMonitorTest extends BaseCuratorDiscoveryTest {
   }
 
   @Test
-  public void testExceptionalLifecycle() throws Exception {
+  public void testNeverStarted() throws Exception {
     // Close on a non-started or failed-to-start monitor should be allowed.
     getGroupMonitor().close();
   }
 
   @Test
+  public void testAlreadyStopped() throws Exception {
+    startGroupMonitor();
+    getGroupMonitor().close();
+    // Multiple closes on a started monitor should be allowed.
+    getGroupMonitor().close();
+  }
+
+  @Test
   public void testNoHosts() throws Exception {
     assertEquals(ImmutableSet.of(), getGroupMonitor().get());