curator-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Blum <dragonsi...@gmail.com>
Subject Re: NamespaceWatcher hashCode and equals still bugging me
Date Wed, 10 Feb 2016 16:30:30 GMT
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 <dragonsinth@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>
> 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 <dragonsinth@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
>> >
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message