BewareMyPower opened a new pull request, #23217:
URL: https://github.com/apache/pulsar/pull/23217
### Background Knowledge
A concurrent hash map (no matter the `ConcurrentOpenHashMap` in Pulsar or
the official `ConcurrentHashMap`) is not a synchronized hash map.
For example, given a `ConcurrentHashMap<Integer, Integer>` object `m`,
```java
synchronized (m) {
m.computeIfAbsent(1, __ -> 100); // [1]
m.computeIfAbsent(2, __ -> 200); // [2]
}
```
```java
m.computeIfAbsent(1, __ -> 300); // [3]
```
If these two code blocks are called in two threads, `[1]->[3]->[2]` is a
possible case.
### Motivation
`SimpleLoadManagerImpl` and `ModularLoadManagerImpl` both maintain the
bundle range cache:
```java
// The 1st key is broker, the 2nd key is namespace
private final ConcurrentOpenHashMap<String, ConcurrentOpenHashMap<String,
ConcurrentOpenHashSet<String>>>
brokerToNamespaceToBundleRange;
```
However, when accessing the `namespace -> bundle` map, it still needs a
synchronized code block:
https://github.com/apache/pulsar/blob/1c495e190b3c569e9dfd44acef2a697c93a1f771/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L591-L595
The code above is a composite operation of `clear()` and multiple
`computeIfAbsent` operations on the `ConcurrentOpenHashMap<String,
ConcurrentOpenHashSet<String>>` object.
So the other place that access this map also requires the same lock even if
the operation itself is thread safe:
https://github.com/apache/pulsar/blob/1c495e190b3c569e9dfd44acef2a697c93a1f771/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java#L882-L886
P.S. `SimpleLoadManagerImpl` does not apply the synchronized block.
However, when accessing `brokerToNamespaceToBundleRange` in the static
methods of `LoadManagerShared`, they are not synchronized. So the access on the
`Map<String, Set<String>>` value is not thread safe.
### Modifications
Add a `BundleRangeCache` abstraction to provide some methods to support
required operations on the bundle range cache.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update
later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
