curator-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Blum <dragonsi...@gmail.com>
Subject Re: PLEASE REVIEW - Major re-work of Watcher wrappers
Date Fri, 05 Feb 2016 17:24:13 GMT
BTW: this is broken on CURATOR-3.0 as well, so it appears to have been
broken for a while.  Maybe I'll have to git bisect...

On Fri, Feb 5, 2016 at 12:22 PM, Scott Blum <dragonsinth@gmail.com> wrote:

> Okay, so I looked into this for a bit, and I hit kind of a wall.  I think
> there is a legit bug/race in TreeCache, and the following patch *should*
> remedy:
>
> diff --git
> a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java
> b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java
> index df4403c..a4a022b 100644
> ---
> a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java
> +++
> b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java
> @@ -303,7 +303,6 @@ public class TreeCache implements Closeable
>          void wasDeleted() throws Exception
>          {
>              ChildData oldChildData = childData.getAndSet(null);
> -
>  client.watches().remove(this).ofType(WatcherType.Any).locally().inBackground().forPath(path);
>              ConcurrentMap<String, TreeNode> childMap =
> children.getAndSet(null);
>              if ( childMap != null )
>              {
> @@ -807,8 +806,16 @@ public class TreeCache implements Closeable
>          case RECONNECTED:
>              try
>              {
> +                outstandingOps.incrementAndGet();
>                  root.wasReconnected();
>                  publishEvent(TreeCacheEvent.Type.CONNECTION_RECONNECTED);
> +                if ( outstandingOps.decrementAndGet() == 0 )
> +                {
> +                    if ( isInitialized.compareAndSet(false, true) )
> +                    {
> +                        publishEvent(TreeCacheEvent.Type.INITIALIZED);
> +                    }
> +                }
>              }
>              catch ( Exception e )
>              {
>
> That should guarantee that the initialized event gets deferred until all
> outstanding refreshes finish.. but it's not.  Something seems to have
> changed under the hood in how background events are getting sent to
> TreeCache, and I don't really understand it yet.  And running the debugger
> seems to affect the timing, like something racy is going on. :(
>
>
> On Fri, Feb 5, 2016 at 11:57 AM, Scott Blum <dragonsinth@gmail.com> wrote:
>
>> Ok, that is kind of weird.  I'll take a look.
>>
>> On Fri, Feb 5, 2016 at 4:58 AM, Jordan Zimmerman <
>> jordan@jordanzimmerman.com> wrote:
>>
>>> No, sorry. The last few lines of the test currently are:
>>>
>>>
>>> assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes());
>>> assertEvent(TreeCacheEvent.Type.INITIALIZED);
>>>
>>> This fails. But, if I switch them it works:
>>>
>>> assertEvent(TreeCacheEvent.Type.INITIALIZED);
>>>
>>> assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes());
>>>
>>> On Feb 5, 2016, at 2:57 AM, Scott Blum <dragonsinth@gmail.com> wrote:
>>>
>>> So you end up with 2 initialized events?
>>>
>>> You mean this?
>>>
>>>          assertEvent(TreeCacheEvent.Type.CONNECTION_RECONNECTED);
>>> +        assertEvent(TreeCacheEvent.Type.INITIALIZED);
>>>          assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me",
>>> "data".getBytes());
>>>          assertEvent(TreeCacheEvent.Type.INITIALIZED);
>>>
>>> Seems weird if there are two, but I can help look.
>>>
>>> On Thu, Feb 4, 2016 at 10:48 PM, Jordan Zimmerman <
>>> jordan@jordanzimmerman.com> wrote:
>>>
>>>> Hey Scott,
>>>>
>>>> In this branch, TestTreeCache.testKilledSession() is failing at:
>>>>
>>>>         assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me",
>>>> "data".getBytes());
>>>>
>>>> However, if I change the two asserts to:
>>>>
>>>>         assertEvent(TreeCacheEvent.Type.INITIALIZED);
>>>>         assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me",
>>>> "data".getBytes());
>>>>
>>>> it works. Does that make any sense?
>>>>
>>>> -Jordan
>>>>
>>>> > On Feb 4, 2016, at 9:23 PM, Jordan Zimmerman <
>>>> jordan@jordanzimmerman.com> wrote:
>>>> >
>>>> > Devs,
>>>> >
>>>> > In trying to fix the bad log message "Failed to find watcher” (which
>>>> turns out to be a ZK client issue), I realize that the NamespaceWatcher and
>>>> WatcherWrapper stuff could be improved. I’m still working on getting all
>>>> tests to pass but I’d appreciate more sets of eyes on this change. Please
>>>> review carefully if you can.
>>>> >
>>>> > https://github.com/apache/curator/pull/131
>>>> >
>>>> > -Jordan
>>>>
>>>>
>>>
>>>
>>
>

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