From 877eb55c93dc284339a86fdaf678ceba0674d808 Mon Sep 17 00:00:00 2001 From: David Trott Date: Sat, 8 Mar 2014 12:29:50 -0800 Subject: [PATCH 1/2] Documented ServiceType's. --- .../curator/x/discovery/ServiceType.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceType.java b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceType.java index a919ace15f..ceeb7d30b2 100644 --- a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceType.java +++ b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceType.java @@ -18,9 +18,39 @@ */ package org.apache.curator.x.discovery; +/** + * The type of the service registration. + * + * STATIC and PERMANENT registrations are created when the x-discovery-server rest service registers another service. + * Where as DYNAMIC registrations are typically used for everything else + * (including the definition of the x-discovery-server rest service itself). + */ public enum ServiceType { + /** + * DYNAMIC registrations (default) are tied to the lifecycle of the creating process. + * + * ServiceDiscoveryImpl.internalRegisterService() maps DYNAMIC registrations to EPHEMERAL nodes as a result they + * don't need to be manually cleaned up. + */ DYNAMIC, + + /** + * STATIC registrations require the caller to regularly re-register the service before a timeout expires. + * + * The timeout is not defined in the curator code (the caller must define the timeout). + * + * However InstanceCleanup.checkService implements the timeout process to expire them after the timeout. + * Even though STATIC registrations are mapped to PERSISTENT zookeeper nodes, they are also cleaned up + * during a clean shutdown of the x-discovery-server rest service ServiceDiscoveryImpl.close(). + */ STATIC, + + /** + * PERMANENT registrations are not tied to the existence of any particular process. + * + * They can be (un)registered through the x-discovery-server rest service, however the process immediately discards + * all knowledge of the registration ServiceDiscoveryImpl.registerService(...) + */ PERMANENT } From baf596723ee006391376ad62df8f957fb98e7846 Mon Sep 17 00:00:00 2001 From: David Trott Date: Sat, 8 Mar 2014 12:35:03 -0800 Subject: [PATCH 2/2] Updated ServiceDiscoveryImpl.registerService so that PERMANENT registrations are not stored within the service. This is to prevent two side effects: When the ServiceDiscoveryImpl class is closed all PERMANENT registrations are deleted. This is not desireable since callers who create PERMANENT registations should expect them to survive until explicitly deleted. PERMANENT registrations should be stateless with respect to a particular service discovery client (assume a cluster of rest services). It should be possible to use any of them to register/unregister. It is not desirable to keep re-registering them whenever the client losses/reconnects to Zookeeper. --- .../details/ServiceDiscoveryImpl.java | 5 +- .../x/discovery/TestServiceDiscovery.java | 93 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java index 9fc2b54d73..bc267fc49b 100644 --- a/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java +++ b/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java @@ -146,7 +146,10 @@ public void close() throws IOException @Override public void registerService(ServiceInstance service) throws Exception { - services.put(service.getId(), service); + if (service.getServiceType() != ServiceType.PERMANENT) + { + services.put(service.getId(), service); + } internalRegisterService(service); } diff --git a/curator-x-discovery/src/test/java/org/apache/curator/x/discovery/TestServiceDiscovery.java b/curator-x-discovery/src/test/java/org/apache/curator/x/discovery/TestServiceDiscovery.java index b615c74a10..bb5c92c936 100644 --- a/curator-x-discovery/src/test/java/org/apache/curator/x/discovery/TestServiceDiscovery.java +++ b/curator-x-discovery/src/test/java/org/apache/curator/x/discovery/TestServiceDiscovery.java @@ -273,4 +273,97 @@ public void testBasic() throws Exception } } } + + @Test + public void testCloseBehavior() throws Exception + { + + TestingServer server = new TestingServer(); + try + { + List closeables = Lists.newArrayList(); + + ServiceInstance testReg = ServiceInstance.builder().payload("thing").name("test").port(10064).build(); + + ServiceInstance dynamicReg = ServiceInstance.builder().payload("dynamic").name("dynamic").port(1111).serviceType(ServiceType.DYNAMIC).build(); + ServiceInstance staticReg = ServiceInstance.builder().payload("static").name("static").port(1112).serviceType(ServiceType.STATIC).build(); + ServiceInstance permanentReg = ServiceInstance.builder().payload("permanent").name("permanent").port(1113).serviceType(ServiceType.PERMANENT).build(); + + try + { + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1)); + closeables.add(client); + client.start(); + + final ServiceDiscoveryBuilder builder = ServiceDiscoveryBuilder.builder(String.class).basePath("/test").client(client).thisInstance(testReg); + ServiceDiscovery discovery = builder.build(); + closeables.add(discovery); + discovery.start(); + + Assert.assertEquals(discovery.queryForNames(), Arrays.asList("test")); + + List> list = Lists.newArrayList(); + list.add(testReg); + Assert.assertEquals(discovery.queryForInstances("test"), list); + + discovery.registerService(dynamicReg); + discovery.registerService(staticReg); + discovery.registerService(permanentReg); + } + finally + { + Collections.reverse(closeables); + for (Closeable c : closeables) + { + CloseableUtils.closeQuietly(c); + } + } + + closeables.clear(); + + try + { + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1)); + closeables.add(client); + client.start(); + + ServiceDiscovery discovery = ServiceDiscoveryBuilder.builder(String.class).basePath("/test").client(client).thisInstance(testReg).build(); + closeables.add(discovery); + discovery.start(); + + Assert.assertEquals(Sets.newHashSet(discovery.queryForNames()), Sets.newHashSet("test", "dynamic", "static", "permanent")); + + List> testRegistrations = Lists.newArrayList(); + testRegistrations.add(testReg); + + final List> emptyList = Lists.newArrayList(); + + + List> permanentRegistrations = Lists.newArrayList(); + permanentRegistrations.add(permanentReg); + + Assert.assertEquals(discovery.queryForInstances("test"), testRegistrations); + Assert.assertEquals(discovery.queryForInstances("dynamic"), emptyList); + Assert.assertEquals(discovery.queryForInstances("static"), emptyList); + Assert.assertEquals(discovery.queryForInstances("permanent"), permanentRegistrations); + + // Ensure Unregister works too. + discovery.unregisterService(permanentReg); + Assert.assertEquals(discovery.queryForInstances("permanent"), emptyList); + } + finally + { + Collections.reverse(closeables); + for (Closeable c : closeables) + { + CloseableUtils.closeQuietly(c); + } + } + + + } finally + { + CloseableUtils.closeQuietly(server); + } + } }