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 <[email protected]> Authored: Mon Jul 4 08:13:33 2016 -0600 Committer: John Sirois <[email protected]> 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());
