You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/14 07:11:30 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #17638: [fix][metadata] Handle session events in a separate thread
lhotari opened a new pull request, #17638:
URL: https://github.com/apache/pulsar/pull/17638
### Motivation
- session listener might block further processing, and therefore it's better to run it in another thread
- mentioned in [another issue discussion](https://github.com/apache/pulsar/pull/17620#issuecomment-1246303451).
- here's an example of a stacktrace happening in a Bookie with the Pulsar metadata store classes:
```
metadata-store-zk-session-watcher-21-1 Waiting Thread ID: 60
jdk.internal.misc.Unsafe.park(Unsafe.java)
java.util.concurrent.locks.LockSupport.park(LockSupport.java:194)
java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1796)
java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3128)
java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1823)
java.util.concurrent.CompletableFuture.join(CompletableFuture.java:2043)
org.apache.pulsar.metadata.coordination.impl.LeaderElectionImpl.close(LeaderElectionImpl.java:233)
org.apache.pulsar.metadata.bookkeeper.PulsarLedgerAuditorManager.close(PulsarLedgerAuditorManager.java:96)
org.apache.bookkeeper.replication.AuditorElector.shutdown(AuditorElector.java:234)
org.apache.bookkeeper.replication.AutoRecoveryMain.shutdown(AutoRecoveryMain.java:154)
org.apache.bookkeeper.replication.AutoRecoveryMain.lambda$new$0(AutoRecoveryMain.java:99)
org.apache.bookkeeper.replication.AutoRecoveryMain$$Lambda$222.onSessionExpired()
org.apache.pulsar.metadata.bookkeeper.PulsarMetadataClientDriver.lambda$setSessionStateListener$0(PulsarMetadataClientDriver.java:68)
org.apache.pulsar.metadata.bookkeeper.PulsarMetadataClientDriver$$Lambda$223.accept()
org.apache.pulsar.metadata.impl.AbstractMetadataStore.lambda$receivedSessionEvent$11(AbstractMetadataStore.java:297)
org.apache.pulsar.metadata.impl.AbstractMetadataStore$$Lambda$342.accept()
java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:807)
org.apache.pulsar.metadata.impl.AbstractMetadataStore.receivedSessionEvent(AbstractMetadataStore.java:295)
org.apache.pulsar.metadata.impl.ZKMetadataStore.receivedSessionEvent(ZKMetadataStore.java:152)
org.apache.pulsar.metadata.impl.ZKMetadataStore$$Lambda$148.accept()
org.apache.pulsar.metadata.impl.ZKSessionWatcher.checkState(ZKSessionWatcher.java:150)
org.apache.pulsar.metadata.impl.ZKSessionWatcher.checkConnectionStatus(ZKSessionWatcher.java:110)
org.apache.pulsar.metadata.impl.ZKSessionWatcher$$Lambda$149.run()
org.apache.pulsar.common.util.Runnables$CatchingAndLoggingRunnable.run(Runnables.java:54)
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
java.lang.Thread.run(Thread.java:834)
```
### Modifications
- handle session events in a separate thread
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #17638: [fix][metadata] Handle session events in a separate thread
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17638:
URL: https://github.com/apache/pulsar/pull/17638#discussion_r970491691
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -409,14 +409,19 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
-
- sessionListeners.forEach(l -> {
- try {
- l.accept(event);
- } catch (Throwable t) {
- log.warn("Error in processing session event", t);
- }
- });
+ try {
+ executor.execute(() -> {
Review Comment:
I noticed that call as well ;-)
So overall I agree with this patch, because it makes it consistent the way we dispatch the events.
But I am not 100% sure that it will fix the original problem
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #17638: [fix][metadata] Handle session events in a separate thread
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17638:
URL: https://github.com/apache/pulsar/pull/17638
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17638: [fix][metadata] Handle session events in a separate thread
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17638:
URL: https://github.com/apache/pulsar/pull/17638#discussion_r970490001
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -409,14 +409,19 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
-
- sessionListeners.forEach(l -> {
- try {
- l.accept(event);
- } catch (Throwable t) {
- log.warn("Error in processing session event", t);
- }
- });
+ try {
+ executor.execute(() -> {
Review Comment:
Good point. However, in AbstractMetadataStore, the other notifications are delivered using a separate thread pool: https://github.com/apache/pulsar/blob/d8c39bcb5becd15944978755c0b02fc52182fa69/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java#L273-L289
This PR makes AbstractMetadataStore more consistent by also delivering session events using this same thread pool.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #17638: [fix][metadata] Handle session events in a separate thread
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17638:
URL: https://github.com/apache/pulsar/pull/17638#discussion_r970482337
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -409,14 +409,19 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
protected void receivedSessionEvent(SessionEvent event) {
isConnected = event.isConnected();
-
- sessionListeners.forEach(l -> {
- try {
- l.accept(event);
- } catch (Throwable t) {
- log.warn("Error in processing session event", t);
- }
- });
+ try {
+ executor.execute(() -> {
Review Comment:
One concern that I have with this patch is that usually in ZooKeeper client applications you leverage the feature that all the events are dispatched in strict order.
Here we are moving the execution of the listeners to another thread, and it actually works as the executor is still single threaded.
But on the other hand.. aren't we moving the problem to the `metadata-store` ?
maybe the problem is to be fixed in `AutoRecoveryMain.... onSessionExpired()` that is performing a blocking operation (depending on a sync ZooKeeper calll) on the main zookeeper thread.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #17638: [fix][metadata] Handle session events in a separate thread
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17638:
URL: https://github.com/apache/pulsar/pull/17638#issuecomment-1246437752
@lhotari @merlimat
BookKeeper fix https://github.com/apache/bookkeeper/pull/3487
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org