dlg99 commented on a change in pull request #10536:
URL: https://github.com/apache/pulsar/pull/10536#discussion_r630363179
##########
File path:
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/coordination/impl/LockManagerImpl.java
##########
@@ -48,7 +49,7 @@
@Slf4j
class LockManagerImpl<T> implements LockManager<T> {
- private final Set<ResourceLockImpl<T>> locks = new HashSet<>();
+ private final Set<ResourceLockImpl<T>> locks = new CopyOnWriteArraySet<>();
Review comment:
> It is best suited for applications in which set sizes generally stay
small, read-only operations vastly outnumber mutative operations, and you need
to prevent interference among threads during traversal.
> Mutative operations (add, set, remove, etc.) are expensive since they
usually entail copying the entire underlying array.
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CopyOnWriteArraySet.html
i am not sure about the size of the set and frequency of acquireLock() just
checking that the copying semantics won't become a perf problem/result in
increased GC vs the alternatives.
##########
File path:
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/coordination/impl/LockManagerImpl.java
##########
@@ -48,7 +49,7 @@
@Slf4j
class LockManagerImpl<T> implements LockManager<T> {
- private final Set<ResourceLockImpl<T>> locks = new HashSet<>();
+ private final Set<ResourceLockImpl<T>> locks = new CopyOnWriteArraySet<>();
Review comment:
Also, probably no longer need `synchronized (LockManagerImpl.this)` in
`acquireLock()` and `locks = new HashSet<>(this.locks);` in `asyncClose()`
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
