You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Randgalt <gi...@git.apache.org> on 2016/02/05 02:49:56 UTC
[GitHub] curator pull request: Major re-work of Watcher wrappers
GitHub user Randgalt opened a pull request:
https://github.com/apache/curator/pull/131
Major re-work of Watcher wrappers
Major re-work of Watcher wrappers. It really isn't necessary to keep a map of watchers. It was originally done so that watcher identity was maintained, but this can be achieved much easier using special-purpose hashCode() and equals() on NamespaceWatcher.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator better-namspace-watcher
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/131.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 #131
----
commit ff8a795e61d0d44622bdbaf2144c25c70e31e864
Author: randgalt <ra...@apache.org>
Date: 2016-02-04T23:57:35Z
Major re-work of Watcher wrappers. It really isn't necessary to keep a map of watchers. It was originally done so that watcher identity was maintained, but this can be achieved much easier using special-purpose hashCode() and equals() on NamespaceWatcher.
commit 3db108102b64255671e472725dc8496a45d8a7e4
Author: randgalt <ra...@apache.org>
Date: 2016-02-05T01:48:17Z
The path should be part of NamespaceWatcher's identity. Also, I can simplify WatcherRemovalManager
----
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52201217
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
--- End diff --
why not just `o instanceof Watcher`?
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52200925
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
+ {
+ return actualWatcher == o;
+ }
+
+ return false;
--- End diff --
Actually, disregard this comment, missed the `curatorWatcher` field
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52201292
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
--- End diff --
Why not just `o instanceof NamespaceWatcher`?
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52200869
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
+ {
+ return actualWatcher == o;
+ }
+
+ return false;
+ }
+
+ @Override
+ public int hashCode()
+ {
+ int result = actualWatcher != null ? actualWatcher.hashCode() : 0;
+ result = 31 * result + unfixedPath.hashCode();
+ result = 31 * result + (curatorWatcher != null ? curatorWatcher.hashCode() : 0);
+ return result;
--- End diff --
Ah, I see what's going on now.
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52199455
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
+ {
+ return actualWatcher == o;
+ }
+
+ return false;
+ }
+
+ @Override
+ public int hashCode()
+ {
+ int result = actualWatcher != null ? actualWatcher.hashCode() : 0;
+ result = 31 * result + unfixedPath.hashCode();
+ result = 31 * result + (curatorWatcher != null ? curatorWatcher.hashCode() : 0);
+ return result;
--- End diff --
shouldn't this just be `return Objects.hashCode(actualWatcher)`?
If the goal is to make this hashCode() and equals() same as underlying watcher, why add in the other components to the hash?
---
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: Major re-work of Watcher wrappers
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/131
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52199739
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
+ {
+ return actualWatcher == o;
+ }
+
+ return false;
--- End diff --
I think you can simplify all this
```
if ( this == o )
{
return true;
}
if ( o instanceof NamespaceWatcher )
{
o = ((NamespaceWatcher)o).actualWatcher;
}
return Objects.equal(this.actualWatcher, o);
```
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52198800
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -240,11 +238,7 @@ public void createContainers(String path) throws Exception
@Override
public void clearWatcherReferences(Watcher watcher)
{
- NamespaceWatcher namespaceWatcher = namespaceWatcherMap.remove(watcher);
- if ( namespaceWatcher != null )
- {
- namespaceWatcher.close();
- }
--- End diff --
Just a question here. The super class doc reads `@deprecated As of ZooKeeper 3.5 Curators recipes will handle removing watcher references when they are no longer used.`
Is this still the most accurate comment? My (loose) understanding is that even the recipes themselves don't need to worry about this as the framework will handle it for them.
---
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: Major re-work of Watcher wrappers
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52203476
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
--- End diff --
I don't think it can actually happen as my main concern is inside ZK. But, for completeness it should be added.
---
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: Major re-work of Watcher wrappers
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52199629
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( Watcher.class.isAssignableFrom(o.getClass()) )
+ {
+ return actualWatcher == o;
+ }
+
+ return false;
+ }
+
+ @Override
+ public int hashCode()
+ {
+ int result = actualWatcher != null ? actualWatcher.hashCode() : 0;
+ result = 31 * result + unfixedPath.hashCode();
+ result = 31 * result + (curatorWatcher != null ? curatorWatcher.hashCode() : 0);
+ return result;
--- End diff --
It needs to match the watcher and the path - that's how ZooKeeper stores it.
---
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: Major re-work of Watcher wrappers
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52199052
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -240,11 +238,7 @@ public void createContainers(String path) throws Exception
@Override
public void clearWatcherReferences(Watcher watcher)
{
- NamespaceWatcher namespaceWatcher = namespaceWatcherMap.remove(watcher);
- if ( namespaceWatcher != null )
- {
- namespaceWatcher.close();
- }
--- End diff --
Recipes have to do a tiny bit of work. They need to get a specially wrapped instance by calling CuratorFramework.newWatcherRemoveCuratorFramework() and then calling removeWatchers() when done.
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52201201
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
+ {
+ NamespaceWatcher watcher = (NamespaceWatcher)o;
+
+ if ( !unfixedPath.equals(watcher.getUnfixedPath()) )
+ {
+ return false;
+ }
+
+ //noinspection SimplifiableIfStatement
+ if ( actualWatcher != null ? !actualWatcher.equals(watcher.actualWatcher) : watcher.actualWatcher != null )
+ {
+ return false;
+ }
+ return curatorWatcher != null ? curatorWatcher.equals(watcher.curatorWatcher) : watcher.curatorWatcher == null;
+ }
+
--- End diff --
Okay, I think there's a missing case here?
```
if ( o instanceof CuratorWatcher )
{
return curatorWatcher == o;
}
```
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52200119
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/Watching.java ---
@@ -48,15 +53,30 @@
{
watcher = null;
watched = false;
+ curatorWatcher = null;
}
Watcher getWatcher(CuratorFrameworkImpl client, String unfixedPath)
{
- if ( (watcher != null) && (client.getWatcherRemovalManager() != null) )
+ NamespaceWatcher namespaceWatcher = null;
+ if ( watcher != null )
+ {
+ namespaceWatcher = new NamespaceWatcher(client, this.watcher, unfixedPath);
+ }
+ else if ( curatorWatcher != null )
{
- return client.getWatcherRemovalManager().add(unfixedPath, watcher);
+ namespaceWatcher = new NamespaceWatcher(client, curatorWatcher, unfixedPath);
}
- return watcher;
+
+ if ( namespaceWatcher != null )
+ {
+ if ( client.getWatcherRemovalManager() != null )
+ {
+ client.getWatcherRemovalManager().add(namespaceWatcher);
+ }
+ }
+
+ return namespaceWatcher;
}
boolean isWatched()
--- End diff --
unrelated to PR, but a name like `needsImplicitWatch` might better explain the purpose
---
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: Major re-work of Watcher wrappers
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52203405
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcher.java ---
@@ -75,4 +89,53 @@ else if ( curatorWatcher != null )
}
}
}
+
+ // specialized equals()/hashCode() that makes this instance equal to the actual watcher
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if ( this == o )
+ {
+ return true;
+ }
+ if ( o == null )
+ {
+ return false;
+ }
+
+ if ( getClass() == o.getClass() )
--- End diff --
This is IntelliJ's auto-gen again the entire equals()/hashCode()/toString() methods are auto generated.
---
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: Major re-work of Watcher wrappers
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/131#discussion_r52200731
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -240,11 +238,7 @@ public void createContainers(String path) throws Exception
@Override
public void clearWatcherReferences(Watcher watcher)
{
- NamespaceWatcher namespaceWatcher = namespaceWatcherMap.remove(watcher);
- if ( namespaceWatcher != null )
- {
- namespaceWatcher.close();
- }
--- End diff --
Cool, maybe update the comment on super decl to reflect this?
---
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.
---