You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by dragonsinth <gi...@git.apache.org> on 2016/01/30 04:49:20 UTC
[GitHub] curator pull request: CURATOR-294: Optimize TreeCache, fix possibl...
GitHub user dragonsinth opened a pull request:
https://github.com/apache/curator/pull/128
CURATOR-294: Optimize TreeCache, fix possible concurrency issue
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dragonsinth/curator CURATOR-294
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/128.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 #128
----
commit 5e995ed7a315fba9f1327706735a7f8c1b417a83
Author: Scott Blum <dr...@apache.org>
Date: 2016-01-28T17:38:57Z
CURATOR-294: Make ChildData immutable; PathChildrenCache replaces instead of mutates.
commit b3e3fcc4848beaa148fc3a5b437e4b19b6e7f325
Author: Scott Blum <dr...@apache.org>
Date: 2016-01-28T17:58:14Z
CURATOR-294: Optimize TreeCache, fix possible concurrency issue
----
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51449251
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java ---
@@ -448,7 +448,10 @@ public boolean clearDataBytes(String fullPath, int ifVersion)
{
if ( (ifVersion < 0) || (ifVersion == data.getStat().getVersion()) )
{
- data.clearData();
+ if ( data.getData() != null )
+ {
+ currentData.replace(fullPath, data, new ChildData(data.getPath(), data.getStat(), null));
+ }
--- End diff --
@Randgalt this is the replacement for `clearData()`. Instead of the ChildData object being mutable, we just replace the ChildData entirely with a copy that has no data.
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51449117
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -92,7 +92,7 @@ public int hashCode()
{
int result = path != null ? path.hashCode() : 0;
result = 31 * result + (stat != null ? stat.hashCode() : 0);
- result = 31 * result + (data != null ? Arrays.hashCode(data.get()) : 0);
+ result = 31 * result + Arrays.hashCode(data);
--- End diff --
Arrays.hashCode handles null internally
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51449278
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -126,12 +126,7 @@ public Stat getStat()
*/
public byte[] getData()
{
- return data.get();
- }
-
- void clearData()
- {
- data.set(null);
+ return data;
--- End diff --
removed that call site, see below
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51449874
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -25,15 +25,15 @@
--- End diff --
done
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51368253
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -25,15 +25,15 @@
--- End diff --
Unused import of AtomicReference
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51453616
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java ---
@@ -448,7 +448,10 @@ public boolean clearDataBytes(String fullPath, int ifVersion)
{
if ( (ifVersion < 0) || (ifVersion == data.getStat().getVersion()) )
{
- data.clearData();
+ if ( data.getData() != null )
+ {
+ currentData.replace(fullPath, data, new ChildData(data.getPath(), data.getStat(), null));
+ }
--- End diff --
OK - I see
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/128
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51448587
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -126,12 +126,7 @@ public Stat getStat()
*/
public byte[] getData()
{
- return data.get();
- }
-
- void clearData()
- {
- data.set(null);
+ return data;
--- End diff --
clearData() needs to remain. PathChildrenCache uses it to release memory. The data is needed temporarily and is cleared afterwards.
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/128#discussion_r51448286
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/ChildData.java ---
@@ -92,7 +92,7 @@ public int hashCode()
{
int result = path != null ? path.hashCode() : 0;
result = 31 * result + (stat != null ? stat.hashCode() : 0);
- result = 31 * result + (data != null ? Arrays.hashCode(data.get()) : 0);
+ result = 31 * result + Arrays.hashCode(data);
--- End diff --
data can legally be null. null checks need to be added throughout this class.
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/128#issuecomment-178076950
Data is an AtomicReference because PathChildrenCache needs to be able to clear it. As I recall, data is needed temporarily and is cleared if the object is marked not to cache data.
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/128#issuecomment-177631319
Other than the unused import, looks good to me.
---
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: CURATOR-294: Optimize TreeCache, fix possibl...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/128#issuecomment-178095680
LGTM
---
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.
---