[
https://issues.apache.org/jira/browse/HDFS-17962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104747#comment-18104747
]
ASF GitHub Bot commented on HDFS-17962:
---------------------------------------
ZanderXu commented on code in PR #8677:
URL: https://github.com/apache/hadoop/pull/8677#discussion_r3782852753
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/top/window/RollingWindowManager.java:
##########
@@ -298,54 +313,23 @@ public TopWindow snapshot(long time) {
private UserCounts getTopUsersForMetric(long time, String metricName,
RollingWindowMap rollingWindows) {
UserCounts topN = new UserCounts(topUsersCnt);
- Iterator<Map.Entry<String, RollingWindow>> iterator =
- rollingWindows.entrySet().iterator();
- while (iterator.hasNext()) {
- Map.Entry<String, RollingWindow> entry = iterator.next();
- String userName = entry.getKey();
- RollingWindow aWindow = entry.getValue();
- long windowSum = aWindow.getSum(time);
- // do the gc here
- if (windowSum == 0) {
- LOG.debug("gc window of metric: {} userName: {}",
- metricName, userName);
- iterator.remove();
- continue;
- }
- LOG.debug("offer window of metric: {} userName: {} sum: {}",
- metricName, userName, windowSum);
- topN.add(new User(userName, windowSum));
+ for (String userName : rollingWindows.keySet()) {
+ rollingWindows.computeIfPresent(userName, (key, window) -> {
Review Comment:
`compute`
Add some comments to show the lock.
> NNTop may lose user metrics during expired window cleanup
> ---------------------------------------------------------
>
> Key: HDFS-17962
> URL: https://issues.apache.org/jira/browse/HDFS-17962
> Project: Hadoop HDFS
> Issue Type: Bug
> Reporter: Yue Wang
> Assignee: Yue Wang
> Priority: Major
> Labels: pull-request-available
>
> NNTop stores a RollingWindow for each (metric, user) pair in a
> ConcurrentHashMap.
> In RollingWindowManager#getTopUsersForMetric(), expired user windows are
> removed when getSum(time) returns 0. Previously, the cleanup used an iterator
> to remove the map entry, while RollingWindowManager#recordMetric() obtained a
> RollingWindow from the map and incremented it outside of any per-key atomic
> operation.
> This allows the following race:
> 1. A recordMetric() thread obtains an existing RollingWindow for a user.
> 2. A snapshot thread observes that the window sum is 0 and removes the
> user-to-window mapping.
> 3. The recordMetric() thread increments the RollingWindow it obtained earlier.
> 4. The increment is applied to an object that is no longer in the map, so the
> metric is lost from subsequent NNTop snapshots.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
