[
https://issues.apache.org/jira/browse/CURATOR-294?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15126590#comment-15126590
]
ASF GitHub Bot commented on CURATOR-294:
----------------------------------------
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.
> 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)
