You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by madrob <gi...@git.apache.org> on 2015/04/10 06:40:25 UTC
[GitHub] curator pull request: CURATOR-164 prevent phantom services
GitHub user madrob opened a pull request:
https://github.com/apache/curator/pull/72
CURATOR-164 prevent phantom services
When ServiceDiscovery reregisters services after a reconnection, it's
possible to add back a service that was already deleted. This is why
we need to remove the service from the cache before deleting it from
zookeeper.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/madrob/curator CURATOR-164
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/72.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #72
----
commit 9dc3ba5a6f0b2b7cfd3254eec5d8c94d89ae4022
Author: Mike Drob <md...@cloudera.com>
Date: 2015-04-10T00:59:31Z
CURATOR-164 prevent phantom services
When ServiceDiscovery reregisters services after a reconnection, it's
possible to add back a service that was already deleted. This is why
we need to remove the service from the cache before deleting it from
zookeeper.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-164 prevent phantom services
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/72#discussion_r28805374
--- Diff: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java ---
@@ -228,15 +234,18 @@ protected void internalRegisterService(ServiceInstance<T> service) throws Ex
public void unregisterService(ServiceInstance<T> service) throws Exception
{
String path = pathForInstance(service.getName(), service.getId());
- try
- {
- client.delete().forPath(path);
- }
- catch ( KeeperException.NoNodeException ignore )
- {
- // ignore
+ synchronized(service) {
--- End diff --
Imagine the following sequence:
T1: reconnect()
T2: unregister(X)
T1: for each service in cache:
T1: register(X)
T2: services.remove(X)
T2: client.delete(X)
T1: client.create(X)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-164 prevent phantom services
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/72#discussion_r28801137
--- Diff: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java ---
@@ -207,9 +207,15 @@ protected void internalRegisterService(ServiceInstance<T> service) throws Ex
{
try
{
- CreateMode mode = (service.getServiceType() == ServiceType.DYNAMIC) ? CreateMode.EPHEMERAL : CreateMode.PERSISTENT;
- client.create().creatingParentsIfNeeded().withMode(mode).forPath(path, bytes);
- isDone = true;
+ synchronized(service) {
+ // Check that the service didn't get deleted while we were reconnecting
+ if (services.containsKey(service.getId()))
--- End diff --
Why is this needed? The test passes without this and logically it seems unnecessary.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-164 prevent phantom services
Posted by busbey <gi...@git.apache.org>.
Github user busbey commented on a diff in the pull request:
https://github.com/apache/curator/pull/72#discussion_r28567933
--- Diff: curator-x-discovery/src/test/java/org/apache/curator/x/discovery/TestServiceDiscovery.java ---
@@ -297,4 +304,70 @@ public void testNoServerOnStart() throws Exception
}
}
}
+
+ // CURATOR-164
+ @Test
+ public void testUnregisterService() throws Exception {
+ final String name = "name";
+
+ final CountDownLatch restartLatch = new CountDownLatch(1);
+ final CountDownLatch serializeLatch = new CountDownLatch(1);
--- End diff --
you made this latch, but then used a sleep call to wait for the serialization to finish. better ot switch to using this latch.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-164 prevent phantom services
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/72#discussion_r28801169
--- Diff: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java ---
@@ -228,15 +234,18 @@ protected void internalRegisterService(ServiceInstance<T> service) throws Ex
public void unregisterService(ServiceInstance<T> service) throws Exception
{
String path = pathForInstance(service.getName(), service.getId());
- try
- {
- client.delete().forPath(path);
- }
- catch ( KeeperException.NoNodeException ignore )
- {
- // ignore
+ synchronized(service) {
--- End diff --
I don't think the sync adds anything. The services are usually copies.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---