You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by bonifaido <gi...@git.apache.org> on 2014/09/17 23:32:13 UTC
[GitHub] curator pull request: ChildData's path check prevents the creation...
GitHub user bonifaido opened a pull request:
https://github.com/apache/curator/pull/45
ChildData's path check prevents the creation of PathChildrenCache.NULL_CHILD_DATA
This makes the whole class unloadable.
I have added a .travis.yml file to enable continuous integration, to spot issues in the future automatically. My dummy fix went into the PR, please ignore that.
Anyhow I'm very happy that the project is now on GitHub!
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bonifaido/curator master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/45.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #45
----
commit f6b1e8905661b8ae6455b87811f0c5f740824961
Author: Nandor Kracser <bo...@gmail.com>
Date: 2014-09-17T20:58:27Z
Enabling Travis
commit 3ef72efc5d9e2044ce3978571f2bc5536400bd14
Author: Nandor Kracser <bo...@gmail.com>
Date: 2014-09-17T21:07:57Z
Fixing PathChildrenCache class initialization
- NULL_CHILD_DATA can be constructed
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56319616
Was wondering why there were heaps of test failures on the master branch! Thanks for the patch. I was thinking of leaving the path validation stuff in the ChildData class, and just modifying the NULL_CHILD_DATA to use the root path (empty path doesn't validate). Does that sound reasonable to you guys?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56056722
Is there a JIRA associated with this PR? If not, can you please create one at https://issues.apache.org/jira/browse/CURATOR
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by bonifaido <gi...@git.apache.org>.
Github user bonifaido commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-60729458
Was fixed in https://github.com/apache/curator/commit/63444f6a2273bd910b33ecb10d1106cb2225e4fb
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56057303
Can you add a unit test that demonstrates the bug? I recently went through and added null checks for a lot of the recipes and would be interested to see why this one should be exempt (not saying either way is right, speaking from curiosity here).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by bonifaido <gi...@git.apache.org>.
Github user bonifaido commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56144966
I prefer the second one as well, taken into account that PatChildrenCache might be deprecated in favor of TreeCache (as I heard :)), and also the null-check makes sense there.
This change broke some tests in the Queue recipes as well, I tried to fix those in another branch: https://github.com/bonifaido/curator/compare/travis
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56135351
Very cool, thanks. Based on your test failure, I ran a git bisect and confirmed that the issue came from commit 05254afe - exactly the one where I added all of the validation. Don't know why it didn't get caught right away... probably missed a maven clean or something like that.
It seems like there are two possible solutions here - remove the null check (which you did), or change NULL_CHILD_DATA to not use a null path (maybe empty string is acceptable here?). It looks like there is nothing special about the object being used, it's just a placeholder. I think I prefer the the second solution, because then users are still protected from undefined/unexpected behaviour, like NPE if they call compareTo().
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by bonifaido <gi...@git.apache.org>.
Github user bonifaido closed the pull request at:
https://github.com/apache/curator/pull/45
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: ChildData's path check prevents the creation...
Posted by bonifaido <gi...@git.apache.org>.
Github user bonifaido commented on the pull request:
https://github.com/apache/curator/pull/45#issuecomment-56082216
Running TestPathChildrenCache gives me:
```
java.lang.ExceptionInInitializerError
at org.apache.curator.framework.recipes.cache.TestPathChildrenCache.testAsyncInitialPopulation(TestPathChildrenCache.java:195)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:84)
at org.testng.internal.Invoker.invokeMethod(Invoker.java:714)
at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1231)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:127)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:111)
at org.testng.TestRunner.privateRun(TestRunner.java:767)
at org.testng.TestRunner.run(TestRunner.java:617)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:348)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:343)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:305)
at org.testng.SuiteRunner.run(SuiteRunner.java:254)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1224)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1149)
at org.testng.TestNG.run(TestNG.java:1057)
at org.testng.remote.RemoteTestNG.run(RemoteTestNG.java:111)
at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:204)
at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:175)
at org.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:125)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:134)
Caused by: java.lang.IllegalArgumentException: Path cannot be null
at org.apache.curator.utils.PathUtils.validatePath(PathUtils.java:48)
at org.apache.curator.framework.recipes.cache.ChildData.<init>(ChildData.java:34)
at org.apache.curator.framework.recipes.cache.PathChildrenCache.<clinit>(PathChildrenCache.java:90)
... 31 more
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---