You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Scott Blum <dr...@gmail.com> on 2016/02/09 17:49:24 UTC
NamespaceWatcher hashCode and equals still bugging me
Hi guys,
I'm a practical guy, not a purist, but the 3.0 implementations of
NamespaceWatcher.hashCode() and equals() are bothering me. The reason I
care is that I want to avoid subtle bugs cropping up.
So here's the problem.
1) equals() is not reflexive between NamespaceWatcher and Watcher
Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following
code might or might not work:
container.add(nw)
container.remove(w)
It depends on whether the underlying container ultimately does
"nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same
problem.
2) hashCode() and equals() inconsistent with each other
Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will
practically never work except by luck.
hashSet.put(nw)
hashSet.contains(w)
Most of the time this will return false, except in the exact case where nw
and w happen to have hashCodes that map into the same bucket, and the
equality check is done the "right" order.
So.... taking a step back, what was underlying motivation for the hashCode
/ equality changes? IE, what's the bigger problem we were trying to solve?
Scott
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Scott Blum <dr...@gmail.com>.
Looks good to me now (fc7404585c455e33efad1df10e2086d880a7e2e9). I've had
a couple of random flakes like this:
Failed tests:
org.apache.curator.framework.recipes.cache.TestPathChildrenCache.testKilledSession(org.apache.curator.framework.recipes.cache.TestPathChildrenCache)
Run 1: TestPathChildrenCache.testKilledSession:642 One or more child
watchers are still registered: [/test]
Run 2: PASS
But I don't *think* it's related to this change, I was seeing them before.
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
OK - I’ll wait to hear from you before doing a release. For now, I’ll update the NamespaceWatcher and submit a PR.
-Jordan
> On Feb 10, 2016, at 12:18 PM, Scott Blum <dr...@gmail.com> wrote:
>
> Sounds great! I have a commit I'm testing right now, that I think cleans up the code and the one identity test. I'm running the full suite now but I can go ahead and push the commit on a branch for you to look at.
>
> On Wed, Feb 10, 2016 at 12:14 PM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
> Doh! Shame on me. I just tested this and it doesn’t work as I intended. In fact there’s no way to make it work. After testing this I’m fine with removing the check against "NamespaceWatcher.equals(raw Watcher).” Also, I’m going to write a TechNote on this to warn people that Curator wraps watchers.
>
> Agreed?
>
> -Jordan
>
>
>> On Feb 10, 2016, at 11:56 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>
>> I think there's a subtlety here that I didn't explain very carefully.
>>
>> Assume w = a raw watcher.
>>
>> I'm 100% fine with new NamespaceWatcher(w).equals(new NamespaceWatcher(w). I think this is the only behavior we're actually relying on. I'm skeptical about new NamespaceWatcher(w).equals(w). Do you have reason to think we're relying on this? Assuming you always wrap a raw Watcher before talking to ZK, all you need is the former, not the latter.
>>
>> On Wed, Feb 10, 2016 at 11:42 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>> Yeah, a weak map would’ve made things easier but the map itself is unnecessary. When I wrote it I wasn’t sure how ZK was implemented internally. Of course, I’m now taking advantage of internal knowledge of ZK but there’s a lot of that in Curator and I feel pretty confident it won’t change anytime soon.
>>
>> NamespaceWatcher is a package protected internal class and is only ever used to wrap passed in Watchers/CuratorWatchers and then passed into ZK. So, the missing comparisons don’t concern me.
>>
>>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
>>
>> This is required and is the “magic” that makes removing the Map possible. This way, I can pass in new NamespaceWatcher instances each time but have them compare equal to the wrapped Watcher. This is vital. What this is doing is creating a proxy that allows a passed in Watcher to be wrapped but appear as equal inside of ZK.
>>
>> -Jordan
>>
>>> On Feb 10, 2016, at 11:30 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>>
>>> Here's where I am right this second. I looked back over commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand it about 90%. I suspect the issue might have been solved by simply having the original NamespaceWatcherMap have weak keys and weak values-- it only had weak values, but again I don't have the 100% view on this.
>>>
>>> That said, the new code seems much cleaner to me. And in general, having NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're only ever passing NamespaceWatcher instances to the ZK layer to add and remove, that seems great.
>>>
>>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher). If we're relying on this behavior anywhere, it's a recipe for problems. If we're NOT relying on this behavior, then we should rip some code out of NamespaceWatcher and have it so that a NamespaceWatcher can only equals another NamespaceWatcher.
>>>
>>> What do you think?
>>>
>>>
>>> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>>> Scott - are you OK with a release or should I wait for more discussion on this issue?
>>>
>>> -Jordan
>>>
>>>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>>>
>>>> Sounds like a job for weak hash map. Will follow up later with more
>>>>
>>>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>>>
>>>> Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
>>>>
>>>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees <http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees>
>>>>
>>>> "if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
>>>>
>>>> Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
>>>>
>>>> Thoughts?
>>>>
>>>> -Jordan
>>>>
>>>>
>>>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>>> >
>>>> > Hi guys,
>>>> >
>>>> > I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
>>>> >
>>>> > So here's the problem.
>>>> >
>>>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>>>> >
>>>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
>>>> >
>>>> > container.add(nw)
>>>> > container.remove(w)
>>>> >
>>>> > It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
>>>> >
>>>> > 2) hashCode() and equals() inconsistent with each other
>>>> >
>>>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
>>>> >
>>>> > hashSet.put(nw)
>>>> > hashSet.contains(w)
>>>> >
>>>> > Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
>>>> >
>>>> >
>>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>>> >
>>>> > Scott
>>>> >
>>>>
>>>
>>>
>>
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Scott Blum <dr...@gmail.com>.
Sounds great! I have a commit I'm testing right now, that I think cleans
up the code and the one identity test. I'm running the full suite now but
I can go ahead and push the commit on a branch for you to look at.
On Wed, Feb 10, 2016 at 12:14 PM, Jordan Zimmerman <
jordan@jordanzimmerman.com> wrote:
> Doh! Shame on me. I just tested this and it doesn’t work as I intended. In
> fact there’s no way to make it work. After testing this I’m fine with
> removing the check against "NamespaceWatcher.equals(raw Watcher).” Also,
> I’m going to write a TechNote on this to warn people that Curator wraps
> watchers.
>
> Agreed?
>
> -Jordan
>
>
> On Feb 10, 2016, at 11:56 AM, Scott Blum <dr...@gmail.com> wrote:
>
> I think there's a subtlety here that I didn't explain very carefully.
>
> Assume w = a raw watcher.
>
> I'm 100% fine with new NamespaceWatcher(w).equals(new
> NamespaceWatcher(w). I think this is the only behavior we're actually
> relying on. I'm skeptical about new NamespaceWatcher(w).equals(w). Do you
> have reason to think we're relying on this? Assuming you always wrap a raw
> Watcher before talking to ZK, all you need is the former, not the latter.
>
> On Wed, Feb 10, 2016 at 11:42 AM, Jordan Zimmerman <
> jordan@jordanzimmerman.com> wrote:
>
>> Yeah, a weak map would’ve made things easier but the map itself is
>> unnecessary. When I wrote it I wasn’t sure how ZK was implemented
>> internally. Of course, I’m now taking advantage of internal knowledge of ZK
>> but there’s a lot of that in Curator and I feel pretty confident it won’t
>> change anytime soon.
>>
>> NamespaceWatcher is a package protected internal class and is only ever
>> used to wrap passed in Watchers/CuratorWatchers and then passed into ZK.
>> So, the missing comparisons don’t concern me.
>>
>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
>>
>>
>> This is required and is the “magic” that makes removing the Map possible.
>> This way, I can pass in new NamespaceWatcher instances each time but have
>> them compare equal to the wrapped Watcher. This is vital. What this is
>> doing is creating a proxy that allows a passed in Watcher to be wrapped but
>> appear as equal inside of ZK.
>>
>> -Jordan
>>
>> On Feb 10, 2016, at 11:30 AM, Scott Blum <dr...@gmail.com> wrote:
>>
>> Here's where I am right this second. I looked back over
>> commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand
>> it about 90%. I *suspect* the issue might have been solved by simply
>> having the original NamespaceWatcherMap have weak keys and weak values-- it
>> only had weak values, but again I don't have the 100% view on this.
>>
>> That said, the new code seems much cleaner to me. And in general, having
>> NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're
>> only ever passing NamespaceWatcher instances to the ZK layer to add and
>> remove, that seems great.
>>
>> The only part that bugs me is having NamespaceWatcher.equals(raw
>> Watcher). If we're relying on this behavior anywhere, it's a recipe for
>> problems. If we're NOT relying on this behavior, then we should rip some
>> code out of NamespaceWatcher and have it so that a NamespaceWatcher can
>> only equals another NamespaceWatcher.
>>
>> What do you think?
>>
>>
>> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <
>> jordan@jordanzimmerman.com> wrote:
>>
>>> Scott - are you OK with a release or should I wait for more discussion
>>> on this issue?
>>>
>>> -Jordan
>>>
>>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dr...@gmail.com> wrote:
>>>
>>> Sounds like a job for weak hash map. Will follow up later with more
>>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jo...@jordanzimmerman.com>
>>> wrote:
>>>
>>>> > So.... taking a step back, what was underlying motivation for the
>>>> hashCode / equality changes? IE, what's the bigger problem we were trying
>>>> to solve?
>>>>
>>>> Before this change, we were maintaining a map from Watcher to
>>>> NamespaceWatcher so that we could track/remove the wrapped watcher. This is
>>>> necessary due to this guarantee of ZooKeeper:
>>>>
>>>>
>>>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees
>>>>
>>>> "if the same watch object is registered for an exists and a getData
>>>> call for the same file and that file is then deleted, the watch object
>>>> would only be invoked once with the deletion notification for the file.”
>>>>
>>>> Given that NamespaceWatcher is an internal wrapper, Curator needs to
>>>> generate the same NamespaceWatcher for a given client’s
>>>> Watcher/CuratorWatcher. The map handled this. In the past, this was
>>>> difficult to manage and had potential memory leaks if the map wasn’t
>>>> managed correctly. It occurred to me that the map isn’t needed if
>>>> NamespaceWatcher could have equality/hash values the same as the Watcher
>>>> that it wraps. My testing proved this.
>>>>
>>>> Thoughts?
>>>>
>>>> -Jordan
>>>>
>>>>
>>>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dr...@gmail.com>
>>>> wrote:
>>>> >
>>>> > Hi guys,
>>>> >
>>>> > I'm a practical guy, not a purist, but the 3.0 implementations of
>>>> NamespaceWatcher.hashCode() and equals() are bothering me. The reason I
>>>> care is that I want to avoid subtle bugs cropping up.
>>>> >
>>>> > So here's the problem.
>>>> >
>>>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>>>> >
>>>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the
>>>> following code might or might not work:
>>>> >
>>>> > container.add(nw)
>>>> > container.remove(w)
>>>> >
>>>> > It depends on whether the underlying container ultimately does
>>>> "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same
>>>> problem.
>>>> >
>>>> > 2) hashCode() and equals() inconsistent with each other
>>>> >
>>>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or
>>>> hashMap will practically never work except by luck.
>>>> >
>>>> > hashSet.put(nw)
>>>> > hashSet.contains(w)
>>>> >
>>>> > Most of the time this will return false, except in the exact case
>>>> where nw and w happen to have hashCodes that map into the same bucket, and
>>>> the equality check is done the "right" order.
>>>> >
>>>> >
>>>> > So.... taking a step back, what was underlying motivation for the
>>>> hashCode / equality changes? IE, what's the bigger problem we were trying
>>>> to solve?
>>>> >
>>>> > Scott
>>>> >
>>>>
>>>>
>>>
>>
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
Doh! Shame on me. I just tested this and it doesn’t work as I intended. In fact there’s no way to make it work. After testing this I’m fine with removing the check against "NamespaceWatcher.equals(raw Watcher).” Also, I’m going to write a TechNote on this to warn people that Curator wraps watchers.
Agreed?
-Jordan
> On Feb 10, 2016, at 11:56 AM, Scott Blum <dr...@gmail.com> wrote:
>
> I think there's a subtlety here that I didn't explain very carefully.
>
> Assume w = a raw watcher.
>
> I'm 100% fine with new NamespaceWatcher(w).equals(new NamespaceWatcher(w). I think this is the only behavior we're actually relying on. I'm skeptical about new NamespaceWatcher(w).equals(w). Do you have reason to think we're relying on this? Assuming you always wrap a raw Watcher before talking to ZK, all you need is the former, not the latter.
>
> On Wed, Feb 10, 2016 at 11:42 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
> Yeah, a weak map would’ve made things easier but the map itself is unnecessary. When I wrote it I wasn’t sure how ZK was implemented internally. Of course, I’m now taking advantage of internal knowledge of ZK but there’s a lot of that in Curator and I feel pretty confident it won’t change anytime soon.
>
> NamespaceWatcher is a package protected internal class and is only ever used to wrap passed in Watchers/CuratorWatchers and then passed into ZK. So, the missing comparisons don’t concern me.
>
>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
>
> This is required and is the “magic” that makes removing the Map possible. This way, I can pass in new NamespaceWatcher instances each time but have them compare equal to the wrapped Watcher. This is vital. What this is doing is creating a proxy that allows a passed in Watcher to be wrapped but appear as equal inside of ZK.
>
> -Jordan
>
>> On Feb 10, 2016, at 11:30 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>
>> Here's where I am right this second. I looked back over commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand it about 90%. I suspect the issue might have been solved by simply having the original NamespaceWatcherMap have weak keys and weak values-- it only had weak values, but again I don't have the 100% view on this.
>>
>> That said, the new code seems much cleaner to me. And in general, having NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're only ever passing NamespaceWatcher instances to the ZK layer to add and remove, that seems great.
>>
>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher). If we're relying on this behavior anywhere, it's a recipe for problems. If we're NOT relying on this behavior, then we should rip some code out of NamespaceWatcher and have it so that a NamespaceWatcher can only equals another NamespaceWatcher.
>>
>> What do you think?
>>
>>
>> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>> Scott - are you OK with a release or should I wait for more discussion on this issue?
>>
>> -Jordan
>>
>>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>>
>>> Sounds like a job for weak hash map. Will follow up later with more
>>>
>>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>>
>>> Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
>>>
>>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees <http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees>
>>>
>>> "if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
>>>
>>> Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
>>>
>>> Thoughts?
>>>
>>> -Jordan
>>>
>>>
>>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>> >
>>> > Hi guys,
>>> >
>>> > I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
>>> >
>>> > So here's the problem.
>>> >
>>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>>> >
>>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
>>> >
>>> > container.add(nw)
>>> > container.remove(w)
>>> >
>>> > It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
>>> >
>>> > 2) hashCode() and equals() inconsistent with each other
>>> >
>>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
>>> >
>>> > hashSet.put(nw)
>>> > hashSet.contains(w)
>>> >
>>> > Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
>>> >
>>> >
>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>> >
>>> > Scott
>>> >
>>>
>>
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
> Do you have reason to think we're relying on this?
Users have direct access to the ZooKeeper handle and could, if they want, set watcher bypassing Curator. If they set allocation a Watcher, w, and set it via Curator it will be wrapped in a NamespaceWatcher. If they pass the same w directly to ZooKeeper it won’t be wrapped. Of course, we could consider this a user error but it’s not documented anywhere. Curator users aren’t aware that their watchers are being wrapped internally.
-Jordan
> On Feb 10, 2016, at 11:56 AM, Scott Blum <dr...@gmail.com> wrote:
>
> I think there's a subtlety here that I didn't explain very carefully.
>
> Assume w = a raw watcher.
>
> I'm 100% fine with new NamespaceWatcher(w).equals(new NamespaceWatcher(w). I think this is the only behavior we're actually relying on. I'm skeptical about new NamespaceWatcher(w).equals(w). Do you have reason to think we're relying on this? Assuming you always wrap a raw Watcher before talking to ZK, all you need is the former, not the latter.
>
> On Wed, Feb 10, 2016 at 11:42 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
> Yeah, a weak map would’ve made things easier but the map itself is unnecessary. When I wrote it I wasn’t sure how ZK was implemented internally. Of course, I’m now taking advantage of internal knowledge of ZK but there’s a lot of that in Curator and I feel pretty confident it won’t change anytime soon.
>
> NamespaceWatcher is a package protected internal class and is only ever used to wrap passed in Watchers/CuratorWatchers and then passed into ZK. So, the missing comparisons don’t concern me.
>
>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
>
> This is required and is the “magic” that makes removing the Map possible. This way, I can pass in new NamespaceWatcher instances each time but have them compare equal to the wrapped Watcher. This is vital. What this is doing is creating a proxy that allows a passed in Watcher to be wrapped but appear as equal inside of ZK.
>
> -Jordan
>
>> On Feb 10, 2016, at 11:30 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>
>> Here's where I am right this second. I looked back over commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand it about 90%. I suspect the issue might have been solved by simply having the original NamespaceWatcherMap have weak keys and weak values-- it only had weak values, but again I don't have the 100% view on this.
>>
>> That said, the new code seems much cleaner to me. And in general, having NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're only ever passing NamespaceWatcher instances to the ZK layer to add and remove, that seems great.
>>
>> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher). If we're relying on this behavior anywhere, it's a recipe for problems. If we're NOT relying on this behavior, then we should rip some code out of NamespaceWatcher and have it so that a NamespaceWatcher can only equals another NamespaceWatcher.
>>
>> What do you think?
>>
>>
>> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>> Scott - are you OK with a release or should I wait for more discussion on this issue?
>>
>> -Jordan
>>
>>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>>
>>> Sounds like a job for weak hash map. Will follow up later with more
>>>
>>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>>
>>> Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
>>>
>>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees <http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees>
>>>
>>> "if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
>>>
>>> Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
>>>
>>> Thoughts?
>>>
>>> -Jordan
>>>
>>>
>>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>> >
>>> > Hi guys,
>>> >
>>> > I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
>>> >
>>> > So here's the problem.
>>> >
>>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>>> >
>>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
>>> >
>>> > container.add(nw)
>>> > container.remove(w)
>>> >
>>> > It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
>>> >
>>> > 2) hashCode() and equals() inconsistent with each other
>>> >
>>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
>>> >
>>> > hashSet.put(nw)
>>> > hashSet.contains(w)
>>> >
>>> > Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
>>> >
>>> >
>>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>> >
>>> > Scott
>>> >
>>>
>>
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Scott Blum <dr...@gmail.com>.
I think there's a subtlety here that I didn't explain very carefully.
Assume w = a raw watcher.
I'm 100% fine with new NamespaceWatcher(w).equals(new NamespaceWatcher(w).
I think this is the only behavior we're actually relying on. I'm skeptical
about new NamespaceWatcher(w).equals(w). Do you have reason to think we're
relying on this? Assuming you always wrap a raw Watcher before talking to
ZK, all you need is the former, not the latter.
On Wed, Feb 10, 2016 at 11:42 AM, Jordan Zimmerman <
jordan@jordanzimmerman.com> wrote:
> Yeah, a weak map would’ve made things easier but the map itself is
> unnecessary. When I wrote it I wasn’t sure how ZK was implemented
> internally. Of course, I’m now taking advantage of internal knowledge of ZK
> but there’s a lot of that in Curator and I feel pretty confident it won’t
> change anytime soon.
>
> NamespaceWatcher is a package protected internal class and is only ever
> used to wrap passed in Watchers/CuratorWatchers and then passed into ZK.
> So, the missing comparisons don’t concern me.
>
> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
>
>
> This is required and is the “magic” that makes removing the Map possible.
> This way, I can pass in new NamespaceWatcher instances each time but have
> them compare equal to the wrapped Watcher. This is vital. What this is
> doing is creating a proxy that allows a passed in Watcher to be wrapped but
> appear as equal inside of ZK.
>
> -Jordan
>
> On Feb 10, 2016, at 11:30 AM, Scott Blum <dr...@gmail.com> wrote:
>
> Here's where I am right this second. I looked back over
> commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand
> it about 90%. I *suspect* the issue might have been solved by simply
> having the original NamespaceWatcherMap have weak keys and weak values-- it
> only had weak values, but again I don't have the 100% view on this.
>
> That said, the new code seems much cleaner to me. And in general, having
> NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're
> only ever passing NamespaceWatcher instances to the ZK layer to add and
> remove, that seems great.
>
> The only part that bugs me is having NamespaceWatcher.equals(raw
> Watcher). If we're relying on this behavior anywhere, it's a recipe for
> problems. If we're NOT relying on this behavior, then we should rip some
> code out of NamespaceWatcher and have it so that a NamespaceWatcher can
> only equals another NamespaceWatcher.
>
> What do you think?
>
>
> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <
> jordan@jordanzimmerman.com> wrote:
>
>> Scott - are you OK with a release or should I wait for more discussion on
>> this issue?
>>
>> -Jordan
>>
>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dr...@gmail.com> wrote:
>>
>> Sounds like a job for weak hash map. Will follow up later with more
>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jo...@jordanzimmerman.com>
>> wrote:
>>
>>> > So.... taking a step back, what was underlying motivation for the
>>> hashCode / equality changes? IE, what's the bigger problem we were trying
>>> to solve?
>>>
>>> Before this change, we were maintaining a map from Watcher to
>>> NamespaceWatcher so that we could track/remove the wrapped watcher. This is
>>> necessary due to this guarantee of ZooKeeper:
>>>
>>>
>>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees
>>>
>>> "if the same watch object is registered for an exists and a getData call
>>> for the same file and that file is then deleted, the watch object would
>>> only be invoked once with the deletion notification for the file.”
>>>
>>> Given that NamespaceWatcher is an internal wrapper, Curator needs to
>>> generate the same NamespaceWatcher for a given client’s
>>> Watcher/CuratorWatcher. The map handled this. In the past, this was
>>> difficult to manage and had potential memory leaks if the map wasn’t
>>> managed correctly. It occurred to me that the map isn’t needed if
>>> NamespaceWatcher could have equality/hash values the same as the Watcher
>>> that it wraps. My testing proved this.
>>>
>>> Thoughts?
>>>
>>> -Jordan
>>>
>>>
>>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dr...@gmail.com> wrote:
>>> >
>>> > Hi guys,
>>> >
>>> > I'm a practical guy, not a purist, but the 3.0 implementations of
>>> NamespaceWatcher.hashCode() and equals() are bothering me. The reason I
>>> care is that I want to avoid subtle bugs cropping up.
>>> >
>>> > So here's the problem.
>>> >
>>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>>> >
>>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the
>>> following code might or might not work:
>>> >
>>> > container.add(nw)
>>> > container.remove(w)
>>> >
>>> > It depends on whether the underlying container ultimately does
>>> "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same
>>> problem.
>>> >
>>> > 2) hashCode() and equals() inconsistent with each other
>>> >
>>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap
>>> will practically never work except by luck.
>>> >
>>> > hashSet.put(nw)
>>> > hashSet.contains(w)
>>> >
>>> > Most of the time this will return false, except in the exact case
>>> where nw and w happen to have hashCodes that map into the same bucket, and
>>> the equality check is done the "right" order.
>>> >
>>> >
>>> > So.... taking a step back, what was underlying motivation for the
>>> hashCode / equality changes? IE, what's the bigger problem we were trying
>>> to solve?
>>> >
>>> > Scott
>>> >
>>>
>>>
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
Yeah, a weak map would’ve made things easier but the map itself is unnecessary. When I wrote it I wasn’t sure how ZK was implemented internally. Of course, I’m now taking advantage of internal knowledge of ZK but there’s a lot of that in Curator and I feel pretty confident it won’t change anytime soon.
NamespaceWatcher is a package protected internal class and is only ever used to wrap passed in Watchers/CuratorWatchers and then passed into ZK. So, the missing comparisons don’t concern me.
> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
This is required and is the “magic” that makes removing the Map possible. This way, I can pass in new NamespaceWatcher instances each time but have them compare equal to the wrapped Watcher. This is vital. What this is doing is creating a proxy that allows a passed in Watcher to be wrapped but appear as equal inside of ZK.
-Jordan
> On Feb 10, 2016, at 11:30 AM, Scott Blum <dr...@gmail.com> wrote:
>
> Here's where I am right this second. I looked back over commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand it about 90%. I suspect the issue might have been solved by simply having the original NamespaceWatcherMap have weak keys and weak values-- it only had weak values, but again I don't have the 100% view on this.
>
> That said, the new code seems much cleaner to me. And in general, having NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're only ever passing NamespaceWatcher instances to the ZK layer to add and remove, that seems great.
>
> The only part that bugs me is having NamespaceWatcher.equals(raw Watcher). If we're relying on this behavior anywhere, it's a recipe for problems. If we're NOT relying on this behavior, then we should rip some code out of NamespaceWatcher and have it so that a NamespaceWatcher can only equals another NamespaceWatcher.
>
> What do you think?
>
>
> On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
> Scott - are you OK with a release or should I wait for more discussion on this issue?
>
> -Jordan
>
>> On Feb 9, 2016, at 1:50 PM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>>
>> Sounds like a job for weak hash map. Will follow up later with more
>>
>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>>
>> Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
>>
>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees <http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees>
>>
>> "if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
>>
>> Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
>>
>> Thoughts?
>>
>> -Jordan
>>
>>
>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
>> >
>> > Hi guys,
>> >
>> > I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
>> >
>> > So here's the problem.
>> >
>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>> >
>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
>> >
>> > container.add(nw)
>> > container.remove(w)
>> >
>> > It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
>> >
>> > 2) hashCode() and equals() inconsistent with each other
>> >
>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
>> >
>> > hashSet.put(nw)
>> > hashSet.contains(w)
>> >
>> > Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
>> >
>> >
>> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>> >
>> > Scott
>> >
>>
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Scott Blum <dr...@gmail.com>.
Here's where I am right this second. I looked back over
commit ff8a795e61d0d44622bdbaf2144c25c70e31e864, and I think I understand
it about 90%. I *suspect* the issue might have been solved by simply
having the original NamespaceWatcherMap have weak keys and weak values-- it
only had weak values, but again I don't have the 100% view on this.
That said, the new code seems much cleaner to me. And in general, having
NamespaceWatcher.equals(NamespaceWatcher) seems 100% legit to me. If we're
only ever passing NamespaceWatcher instances to the ZK layer to add and
remove, that seems great.
The only part that bugs me is having NamespaceWatcher.equals(raw Watcher).
If we're relying on this behavior anywhere, it's a recipe for problems. If
we're NOT relying on this behavior, then we should rip some code out of
NamespaceWatcher and have it so that a NamespaceWatcher can only equals
another NamespaceWatcher.
What do you think?
On Wed, Feb 10, 2016 at 8:48 AM, Jordan Zimmerman <
jordan@jordanzimmerman.com> wrote:
> Scott - are you OK with a release or should I wait for more discussion on
> this issue?
>
> -Jordan
>
> On Feb 9, 2016, at 1:50 PM, Scott Blum <dr...@gmail.com> wrote:
>
> Sounds like a job for weak hash map. Will follow up later with more
> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jo...@jordanzimmerman.com>
> wrote:
>
>> > So.... taking a step back, what was underlying motivation for the
>> hashCode / equality changes? IE, what's the bigger problem we were trying
>> to solve?
>>
>> Before this change, we were maintaining a map from Watcher to
>> NamespaceWatcher so that we could track/remove the wrapped watcher. This is
>> necessary due to this guarantee of ZooKeeper:
>>
>>
>> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees
>>
>> "if the same watch object is registered for an exists and a getData call
>> for the same file and that file is then deleted, the watch object would
>> only be invoked once with the deletion notification for the file.”
>>
>> Given that NamespaceWatcher is an internal wrapper, Curator needs to
>> generate the same NamespaceWatcher for a given client’s
>> Watcher/CuratorWatcher. The map handled this. In the past, this was
>> difficult to manage and had potential memory leaks if the map wasn’t
>> managed correctly. It occurred to me that the map isn’t needed if
>> NamespaceWatcher could have equality/hash values the same as the Watcher
>> that it wraps. My testing proved this.
>>
>> Thoughts?
>>
>> -Jordan
>>
>>
>> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dr...@gmail.com> wrote:
>> >
>> > Hi guys,
>> >
>> > I'm a practical guy, not a purist, but the 3.0 implementations of
>> NamespaceWatcher.hashCode() and equals() are bothering me. The reason I
>> care is that I want to avoid subtle bugs cropping up.
>> >
>> > So here's the problem.
>> >
>> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
>> >
>> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the
>> following code might or might not work:
>> >
>> > container.add(nw)
>> > container.remove(w)
>> >
>> > It depends on whether the underlying container ultimately does
>> "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same
>> problem.
>> >
>> > 2) hashCode() and equals() inconsistent with each other
>> >
>> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap
>> will practically never work except by luck.
>> >
>> > hashSet.put(nw)
>> > hashSet.contains(w)
>> >
>> > Most of the time this will return false, except in the exact case where
>> nw and w happen to have hashCodes that map into the same bucket, and the
>> equality check is done the "right" order.
>> >
>> >
>> > So.... taking a step back, what was underlying motivation for the
>> hashCode / equality changes? IE, what's the bigger problem we were trying
>> to solve?
>> >
>> > Scott
>> >
>>
>>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
Scott - are you OK with a release or should I wait for more discussion on this issue?
-Jordan
> On Feb 9, 2016, at 1:50 PM, Scott Blum <dr...@gmail.com> wrote:
>
> Sounds like a job for weak hash map. Will follow up later with more
>
> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jordan@jordanzimmerman.com <ma...@jordanzimmerman.com>> wrote:
> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>
> Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
>
> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees <http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees>
>
> "if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
>
> Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
>
> Thoughts?
>
> -Jordan
>
>
> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dragonsinth@gmail.com <ma...@gmail.com>> wrote:
> >
> > Hi guys,
> >
> > I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
> >
> > So here's the problem.
> >
> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
> >
> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
> >
> > container.add(nw)
> > container.remove(w)
> >
> > It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
> >
> > 2) hashCode() and equals() inconsistent with each other
> >
> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
> >
> > hashSet.put(nw)
> > hashSet.contains(w)
> >
> > Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
> >
> >
> > So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
> >
> > Scott
> >
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Scott Blum <dr...@gmail.com>.
Sounds like a job for weak hash map. Will follow up later with more
On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <jo...@jordanzimmerman.com>
wrote:
> > So.... taking a step back, what was underlying motivation for the
> hashCode / equality changes? IE, what's the bigger problem we were trying
> to solve?
>
> Before this change, we were maintaining a map from Watcher to
> NamespaceWatcher so that we could track/remove the wrapped watcher. This is
> necessary due to this guarantee of ZooKeeper:
>
>
> http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees
>
> "if the same watch object is registered for an exists and a getData call
> for the same file and that file is then deleted, the watch object would
> only be invoked once with the deletion notification for the file.”
>
> Given that NamespaceWatcher is an internal wrapper, Curator needs to
> generate the same NamespaceWatcher for a given client’s
> Watcher/CuratorWatcher. The map handled this. In the past, this was
> difficult to manage and had potential memory leaks if the map wasn’t
> managed correctly. It occurred to me that the map isn’t needed if
> NamespaceWatcher could have equality/hash values the same as the Watcher
> that it wraps. My testing proved this.
>
> Thoughts?
>
> -Jordan
>
>
> > On Feb 9, 2016, at 11:49 AM, Scott Blum <dr...@gmail.com> wrote:
> >
> > Hi guys,
> >
> > I'm a practical guy, not a purist, but the 3.0 implementations of
> NamespaceWatcher.hashCode() and equals() are bothering me. The reason I
> care is that I want to avoid subtle bugs cropping up.
> >
> > So here's the problem.
> >
> > 1) equals() is not reflexive between NamespaceWatcher and Watcher
> >
> > Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the
> following code might or might not work:
> >
> > container.add(nw)
> > container.remove(w)
> >
> > It depends on whether the underlying container ultimately does
> "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same
> problem.
> >
> > 2) hashCode() and equals() inconsistent with each other
> >
> > Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap
> will practically never work except by luck.
> >
> > hashSet.put(nw)
> > hashSet.contains(w)
> >
> > Most of the time this will return false, except in the exact case where
> nw and w happen to have hashCodes that map into the same bucket, and the
> equality check is done the "right" order.
> >
> >
> > So.... taking a step back, what was underlying motivation for the
> hashCode / equality changes? IE, what's the bigger problem we were trying
> to solve?
> >
> > Scott
> >
>
>
Re: NamespaceWatcher hashCode and equals still bugging me
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
> So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
Before this change, we were maintaining a map from Watcher to NamespaceWatcher so that we could track/remove the wrapped watcher. This is necessary due to this guarantee of ZooKeeper:
http://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html#sc_WatchGuarantees
"if the same watch object is registered for an exists and a getData call for the same file and that file is then deleted, the watch object would only be invoked once with the deletion notification for the file.”
Given that NamespaceWatcher is an internal wrapper, Curator needs to generate the same NamespaceWatcher for a given client’s Watcher/CuratorWatcher. The map handled this. In the past, this was difficult to manage and had potential memory leaks if the map wasn’t managed correctly. It occurred to me that the map isn’t needed if NamespaceWatcher could have equality/hash values the same as the Watcher that it wraps. My testing proved this.
Thoughts?
-Jordan
> On Feb 9, 2016, at 11:49 AM, Scott Blum <dr...@gmail.com> wrote:
>
> Hi guys,
>
> I'm a practical guy, not a purist, but the 3.0 implementations of NamespaceWatcher.hashCode() and equals() are bothering me. The reason I care is that I want to avoid subtle bugs cropping up.
>
> So here's the problem.
>
> 1) equals() is not reflexive between NamespaceWatcher and Watcher
>
> Assuming you have a NamespaceWatcher nw wrapping a Watcher w, the following code might or might not work:
>
> container.add(nw)
> container.remove(w)
>
> It depends on whether the underlying container ultimately does "nw.equals(w)" or "w.equals(nw)". Set.contains() would have the same problem.
>
> 2) hashCode() and equals() inconsistent with each other
>
> Because nw.hashCode() != w.hashCode(), lookups in a hashSet or hashMap will practically never work except by luck.
>
> hashSet.put(nw)
> hashSet.contains(w)
>
> Most of the time this will return false, except in the exact case where nw and w happen to have hashCodes that map into the same bucket, and the equality check is done the "right" order.
>
>
> So.... taking a step back, what was underlying motivation for the hashCode / equality changes? IE, what's the bigger problem we were trying to solve?
>
> Scott
>