[
https://issues.apache.org/jira/browse/CURATOR-294?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15126596#comment-15126596
]
ASF GitHub Bot commented on CURATOR-294:
----------------------------------------
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.
> Optimize TreeCache, fix possible concurrency issue
> --------------------------------------------------
>
> Key: CURATOR-294
> URL: https://issues.apache.org/jira/browse/CURATOR-294
> Project: Apache Curator
> Issue Type: Improvement
> Components: Recipes
> Reporter: Nick Hill
> Assignee: Scott Blum
> Fix For: 3.0.1, 2.9.2
>
>
> Hi, I have been looking at the TreeCache impl and have some questions.
> It doesn't look right to me that there's separate atomic refs for a node's
> data and stat. It seems the stat in a ChildData object obtained from
> getCurrentData() might not correspond to the data that it's with. This could
> be problematic when doing conditional state changes given assumptions about
> that data.
> An obvious and simple solution to this would be to have a single
> AtomicReference<ChildData> field instead, which would have the additional
> significant benefit of eliminating ChildData obj creation on every cache
> access. PathChildrenCache works this way, but my understanding was that
> TreeCache is intended to be a (more flexible) replacement.
> Furthermore I'd propose that the data field of ChildData be just a final
> byte[] instead of an AtomicReference. This would avoid needing two volatile
> reads to get to the data, and mean that "sharing" these (per above) is a bit
> safer. The ChildData byte[] AtomicReference is only used by
> PathChildrenCache.clearDataBytes() (not currently used by TreeCache at all),
> and that capability could be easily maintained by having PathChildrenCache
> use it's own simple subclass of ChildData containing the atomic reference.
> If similar capability were to be added to TreeCache, I'd suggest it would be
> better to just replace the node's ChildData object with a copy that has the
> byte[] field nulled out (but same stat ref).
> I'm fairly new to the code so apologies if there's something I've
> missed/misunderstood! But if there is agreement, I'd also be happy to prepare
> a PR.
> Regards,
> Nick
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
