Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In LockManager use concurrent hash map to handle locks notifiications #10680

Merged
merged 2 commits into from
May 22, 2021

Conversation

merlimat
Copy link
Contributor

Motivation

In LockManagerImpl, the notification will use the locks set to re-check the status of the lock against the metadata store. The access needs to be synchronized because new locks could be added again to the list from other threads.

That leads to these exceptions:

08:50:22.921 [metadata-store-5-1] ERROR org.apache.pulsar.metadata.impl.AbstractMetadataStore - Failed to process metadata store notification
java.util.ConcurrentModificationException: null
	at java.util.HashMap$KeySpliterator.forEachRemaining(HashMap.java:1610) ~[?:?]
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
	at org.apache.pulsar.metadata.coordination.impl.LockManagerImpl.handleDataNotification(LockManagerImpl.java:119) ~[classes/:?]
	at org.apache.pulsar.metadata.impl.AbstractMetadataStore.lambda$receivedNotification$0(AbstractMetadataStore.java:149) ~[classes/:?]
	at java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:804) ~[?:?]
	at org.apache.pulsar.metadata.impl.AbstractMetadataStore.lambda$receivedNotification$1(AbstractMetadataStore.java:147) ~[classes/:?]
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1771) [?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.63.Final.jar:4.1.63.Final]
	at java.lang.Thread.run(Thread.java:830) [?:?]

Additionally, we should use a Map instead of a Set to avoid going through the list when one single node is deleted.

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label May 22, 2021
@merlimat merlimat added this to the 2.8.0 milestone May 22, 2021
@merlimat merlimat self-assigned this May 22, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@merlimat merlimat changed the title In LockManager use sync block to handle locks notifiications In LockManager use concurrent hash map to handle locks notifiications May 22, 2021
@merlimat merlimat merged commit e1b2a9a into apache:master May 22, 2021
@merlimat merlimat deleted the lock-manager-sync branch May 22, 2021 16:32
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
…apache#10680)

* In LockManager use sync block to handle locks notifiications

* Use ConcurrentHashMap instead of sync block
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…apache#10680)

* In LockManager use sync block to handle locks notifiications

* Use ConcurrentHashMap instead of sync block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants