I’ll have a look.
> On May 26, 2016, at 10:04 PM, Cameron McKenzie <[email protected]> wrote:
>
> I'm still seeing 6 failed tests that seem related to the same stuff after
> merging your fix:
>
> Failed tests:
> org.apache.curator.framework.recipes.cache.TestPathChildrenCache.testBasics(org.apache.curator.framework.recipes.cache.TestPathChildrenCache)
> Run 1: TestPathChildrenCache.testBasics:863 One or more child watchers
> are still registered: [/test]
> Run 2: TestPathChildrenCache.testBasics:863 One or more child watchers
> are still registered: [/test]
>
> org.apache.curator.framework.recipes.cache.TestPathChildrenCache.testBasicsOnTwoCachesWithSameExecutor(org.apache.curator.framework.recipes.cache.TestPathChildrenCache)
> Run 1: TestPathChildrenCache.testBasicsOnTwoCachesWithSameExecutor:934
> One or more child watchers are still registered: [/test]
> Run 2: TestPathChildrenCache.testBasicsOnTwoCachesWithSameExecutor:934
> One or more child watchers are still registered: [/test]
>
> org.apache.curator.framework.recipes.cache.TestPathChildrenCache.testEnsurePath(org.apache.curator.framework.recipes.cache.TestPathChildrenCache)
> Run 1: TestPathChildrenCache.testEnsurePath:363 One or more child
> watchers are still registered: [/one/two/three]
> Run 2: TestPathChildrenCache.testEnsurePath:363 One or more child
> watchers are still registered: [/one/two/three]
>
> TestInterProcessSemaphoreCluster.testCluster:294 expected [true] but
> found [false]
> org.apache.curator.framework.recipes.shared.TestSharedCount.testMultiClientVersioned(org.apache.curator.framework.recipes.shared.TestSharedCount)
> Run 1: PASS
> Run 2: TestSharedCount.testMultiClientVersioned:256 One or more data
> watchers are still registered: [/count]
>
> org.apache.curator.framework.recipes.shared.TestSharedCount.testSimple(org.apache.curator.framework.recipes.shared.TestSharedCount)
> Run 1: TestSharedCount.testSimple:174 One or more data watchers are still
> registered: [/count]
> Run 2: TestSharedCount.testSimple:174 One or more data watchers are still
> registered: [/count]
>
>
> Tests run: 491, Failures: 6, Errors: 0, Skipped: 0
>
>
> On Fri, May 27, 2016 at 3:30 AM, Jordan Zimmerman <
> [email protected]> wrote:
>
>> I see the problem. The fix is not simple though so I’ll spend some time on
>> it. The TL;DR is that exists watchers are still supposed to get set when
>> there is a KeeperException.NoNode and the code isn’t handling it. But,
>> while I was looking at the code I realized there are some significant
>> additional problems. Curator, here, is trying to mirror what ZooKeeper does
>> internally which is insanely complicated. In hindsight, the whole ZK
>> watcher mechanism should’ve been decoupled from the mutator APIs. But, of
>> course, that’s easy for me to say now.
>>
>> -Jordan
>>
>>> On May 26, 2016, at 1:10 AM, Cameron McKenzie <[email protected]>
>> wrote:
>>>
>>> Thanks Scott,
>>> Those tests are now passing for me.
>>>
>>> Jordan, testNodeCache:testBasics() is failing consistently on the 3.0
>>> branch. It appears that this is actually potentially a bug in the
>>> NodeCache. It ends up leaking a Watcher reference. I've had a quick look
>>> through, but I haven't dived in in any detail. It's the
>>> WatcherRemovalManager stuff I think. If you've got time, can you have a
>>> look? If not, let me know and I'll do some more digging.
>>>
>>> cheers
>>>
>>> On Thu, May 26, 2016 at 11:47 AM, Cameron McKenzie <
>> [email protected]>
>>> wrote:
>>>
>>>> Thanks Scott.
>>>>
>>>> Push the fix to master and merge it into 3.0.
>>>>
>>>> Then I guess, I'll push new versions of 2.11 and 3.2 onto Nexus.
>>>> cheers
>>>>
>>>> On Thu, May 26, 2016 at 11:44 AM, Scott Blum <[email protected]>
>>>> wrote:
>>>>
>>>>> Alright, I have a fix, but it wants to be applied to both master and
>> 3.0.
>>>>> Where should I push the fix?
>>>>>
>>>>> On Wed, May 25, 2016 at 6:10 PM, Cameron McKenzie <
>> [email protected]
>>>>>>
>>>>> wrote:
>>>>>
>>>>>> Thanks Scott,
>>>>>> If you just checkout the CURATOR-3.0 branch, they are failing there.
>>>>>> cheers
>>>>>>
>>>>>> On Thu, May 26, 2016 at 2:06 AM, Scott Blum <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>>> Sure, what SHA are they failing at Cam?
>>>>>>>
>>>>>>> On Wed, May 25, 2016 at 9:36 AM, Jordan Zimmerman <
>>>>>>> [email protected]> wrote:
>>>>>>>
>>>>>>>> Scott can you take a look?
>>>>>>>>
>>>>>>>> -Jordan
>>>>>>>>
>>>>>>>>> On May 25, 2016, at 4:35 AM, Cameron McKenzie <
>>>>>> [email protected]>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Tree cache tests are still failing. I've tried a few times but no
>>>>>> love:
>>>>>>>>>
>>>>>>>>> TestTreeCacheEventOrdering>TestEventOrdering.testEventOrdering:151
>>>>>>>> actual 6
>>>>>>>>> expected -31:
>>>>>>>>>
>>>>>>>>> I will have a look into what's going on in the morning. Given that
>>>>>>> these
>>>>>>>>> may take some messing about to fix up, do we just want to vote on
>>>>>>> 2.11.0
>>>>>>>>> separately, as that is all ready to go?
>>>>>>>>> cheers
>>>>>>>>>
>>>>>>>>> On Wed, May 25, 2016 at 5:34 PM, Jordan Zimmerman <
>>>>>>>>> [email protected]> wrote:
>>>>>>>>>
>>>>>>>>>> Great news. Thanks.
>>>>>>>>>>
>>>>>>>>>> ====================
>>>>>>>>>> Jordan Zimmerman
>>>>>>>>>>
>>>>>>>>>>> On May 25, 2016, at 12:37 AM, Cameron McKenzie <
>>>>>>> [email protected]
>>>>>>>>>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> I have fixed up the test case, all good now.
>>>>>>>>>>>
>>>>>>>>>>> On Wed, May 25, 2016 at 1:45 PM, Cameron McKenzie <
>>>>>>>>>> [email protected]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Looks like it was introduced with the schema validation stuff.
>>>>> It
>>>>>>> now
>>>>>>>>>> does
>>>>>>>>>>>> an ACL check prior to the backgrounding call. Because the unit
>>>>>> test
>>>>>>>>>> uses a
>>>>>>>>>>>> bogus ACL provider it just throws an exception
>>>>>>>>>>>>
>>>>>>>>>>>> final String adjustedPath =
>>>>>>>>>>>> adjustPath(client.fixForNamespace(givenPath,
>>>>>>>>>> createMode.isSequential()));
>>>>>>>>>>>> List<ACL> aclList = acling.getAclList(adjustedPath);
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>> client.getSchemaSet().getSchema(givenPath).validateCreate(createMode,
>>>>>>>>>> data,
>>>>>>>>>>>> aclList);
>>>>>>>>>>>>
>>>>>>>>>>>> String returnPath = null;
>>>>>>>>>>>> if ( backgrounding.inBackground() )
>>>>>>>>>>>> {
>>>>>>>>>>>> pathInBackground(adjustedPath, data, givenPath);
>>>>>>>>>>>>
>>>>>>>>>>>> So, I guess the answer is to get the test to force a failure
>>>>> in a
>>>>>>>>>>>> different way. With the UnhandledErrorListener, the
>>>>> expectation is
>>>>>>>> that
>>>>>>>>>>>> this only gets called on backgrounding operations?
>>>>>>>>>>>> cheers
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, May 25, 2016 at 1:39 PM, Cameron McKenzie <
>>>>>>>>>> [email protected]>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Same on the master branch, but it passes there, so maybe
>>>>>> something
>>>>>>>> has
>>>>>>>>>>>>> legitimately broken the test. Will let you know if I get
>>>>> stuck.
>>>>>>>>>>>>> cheers
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, May 25, 2016 at 1:36 PM, Jordan Zimmerman <
>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Let me know if you need help.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It might be a bad merge. Have you compared it to the master
>>>>>>> branch?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -JZ
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On May 24, 2016, at 10:31 PM, Cameron McKenzie <
>>>>>>>>>> [email protected]>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Guys,
>>>>>>>>>>>>>>> There's a test TestFrameworkBackground:testErrorListener
>>>>> that
>>>>>> is
>>>>>>>>>>>>>> failing
>>>>>>>>>>>>>>> consistently on the CURATOR-3.0 branch.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I can't see how it has ever worked. It seems to try and
>>>>> provoke
>>>>>>> an
>>>>>>>>>>>>>> error
>>>>>>>>>>>>>>> via a bad ACL provider.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But this ACL provider is called by the CreateBuilderImpl
>>>>> prior
>>>>>> to
>>>>>>>> the
>>>>>>>>>>>>>>> backgrounding call, which means that the exception that it
>>>>>> throws
>>>>>>>>>>>>>> happens
>>>>>>>>>>>>>>> in the main Thread of the unit test. So, it just throws an
>>>>>>>>>>>>>>> UnsupportedOperationException which is propogated up the
>>>>> stack
>>>>>> at
>>>>>>>>>> which
>>>>>>>>>>>>>>> point the unit test fails.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So, I will look at fixing this, but I just don't understand
>>>>> how
>>>>>>> it
>>>>>>>>>> ever
>>>>>>>>>>>>>>> worked?
>>>>>>>>>>>>>>> cheers
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>
>>
