[
https://issues.apache.org/jira/browse/HDFS-17777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072827#comment-18072827
]
ASF GitHub Bot commented on HDFS-17777:
---------------------------------------
balodesecurity commented on PR #8325:
URL: https://github.com/apache/hadoop/pull/8325#issuecomment-4229268852
Thanks @ZanderXu for the review!
The `ConcurrentHashMap + synchronized(set)` approach is cleaner for pure
read/write separation, but I ran into a subtle race in `remove()`: when the
last block is removed from a set, we remove the empty set from the map. With
`ConcurrentHashMap`, a concurrent `add()` could:
1. Get a reference to the set via `computeIfAbsent` (set still in map)
2. Get preempted while another thread removes the last element and calls
`map.remove(uuid, set)`
3. Resume and add a block to a set that is no longer in the map — silently
losing the block
To fix this, `add()` would need to verify the set is still in the map after
acquiring `synchronized(set)` and retry if not — which adds a loop.
The `ReentrantReadWriteLock` approach avoids this entirely since the whole
map + set operation is atomic under the write lock.
Happy to switch to `ConcurrentHashMap + synchronized(set)` if you have a
preferred pattern for handling the empty-set removal race — or if it's
acceptable to not remove empty sets from the map. Please let me know!
> Improve ExcessRedundancyMap locking semantics
> ---------------------------------------------
>
> Key: HDFS-17777
> URL: https://issues.apache.org/jira/browse/HDFS-17777
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: namenode
> Affects Versions: 3.4.0, 3.3.6, 3.4.1
> Reporter: William Montaz
> Priority: Major
> Labels: pull-request-available
> Attachments: Capture d’écran 2025-04-30 à 14.32.41.png
>
>
> ExcessRedundancyMap introduce in HDFS-9838 uses synchronized keyword for
> threadsafety. However prior to introduce this class, the operations such as
> contains were made without a new lock, they were called inside BlockManager
> with verification that the fsNamesystem lock was held in read or write mode
> depending on the situation.
>
> ExcessRedundancyMap now forces all thread to grab the same exclusive lock,
> even if in general a lot more read are performed on the class.
>
> By using a ReentrantReadWriteLock for ExcessRedundancyMap we could improve
> throughput of the namenode. Another approach could be to introduce the same
> asserts on namesystem lock as it seems those methods are always called in the
> context of the FSNamesystem lock.
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
