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 < [email protected]> 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 <[email protected]> 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 < > [email protected]> 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 <[email protected]> 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 < >> [email protected]> 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 <[email protected]> wrote: >>> >>> Sounds like a job for weak hash map. Will follow up later with more >>> On Feb 9, 2016 12:01 PM, "Jordan Zimmerman" <[email protected]> >>> 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 <[email protected]> >>>> 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 >>>> > >>>> >>>> >>> >> >> > >
