[
https://issues.apache.org/jira/browse/HADOOP-12593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15032651#comment-15032651
]
Sangjin Lee commented on HADOOP-12593:
--------------------------------------
{quote}
Hmm. The declaration of rootData has a comment saying that it is protected by
the Statistics lock. There's also a comment near reset talking about the
locking, commenting that "Both reads and writes to rootData are done under the
lock, so we're free to modify rootData from any thread that holds the lock".
Are other places we could add comments to make it clearer? To be honest, I
think the locking here is better documented than most places in our code base.
{quote}
I totally agree that it is better documented than other places in our code base
(and the code is correct). It is about a couple of places we can touch up to
make it crystal clear. The only minor nits I had were
- the single-writer assumption has one exception, which is Statistics.rootData:
this could have been documented in StatisticsData so that one can connect the
dots between the single-writer assumption and rootData quickly
- The access to rootData in Statistics.reset() is safe because visitAll() is
synchronized, but it may be easy to miss (I did when I went over it quickly).
As you mentioned, there is the javadoc comment there mentioning the lock, but a
quick mention to visitAll() would have been even better.
> multiple "volatile long" field declarations exist in the Hadoop codebase
> ------------------------------------------------------------------------
>
> Key: HADOOP-12593
> URL: https://issues.apache.org/jira/browse/HADOOP-12593
> Project: Hadoop Common
> Issue Type: Bug
> Affects Versions: 2.8.0
> Reporter: Steve Loughran
> Priority: Minor
>
> If you get your IDE to scan for "volatile long", you find 20-30 entries.
> Volatile operations on `long` variables are not guaranteed to be atomic, so
> these usages can be vulnerable to race conditions generating invalid data.
> they need to be replaced by AtomicLong references, except in the specific
> case that you want performance values for statistics, and are prepared to
> take the risk
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
