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 2021/04/28 05:10:31 UTC
[GitHub] [pulsar] lhotari opened a new pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
lhotari opened a new pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413
### Motivation
There are concurrency issues in `PersistentDispatcherMultipleConsumers` and `PersistentStreamingDispatcherMultipleConsumers` classes. Some symptoms or previous fixes are #5311, #6054, #6255, #7266, #9789 .
Currently `PersistentStreamingDispatcherMultipleConsumers.readMoreEntries` method isn't synchronized:
https://github.com/apache/pulsar/blob/f2d72c9fc13a33df584ec1bd96a4c147774b858d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStreamingDispatcherMultipleConsumers.java#L137-L140
In one location the method is called within a synchronized block like this:
https://github.com/apache/pulsar/blob/31f831574c3a65bae0e76801facaf2deb0b17fbb/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java#L191-L205
Another location where the `readEntries` call is explicitly wrapped in a synchronized block
https://github.com/apache/pulsar/blob/875262a0ae2fba82a6dd7d46dd2467d675b0d9c3/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L612-L621
Synchronization is missing in this location (and a few other locations):
https://github.com/apache/pulsar/blob/a1cebdb3e7cc3b8ee3ce567058182e7d31c762d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2439-L2451
It seems that it would be more consistent to make the `readMoreEntries` method a `synchronized` method.
### Modifications
- make `readEntriesMethod` in `PersistentDispatcherMultipleConsumers` and `PersistentStreamingDispatcherMultipleConsumers` classes a `synchronized` method.
--
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:
users@infra.apache.org
[GitHub] [pulsar] lhotari removed a comment on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828175584
--
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:
users@infra.apache.org
[GitHub] [pulsar] devinbost commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828179872
You know you've got my support, though I do wonder if there was a specific reason why it wasn't synchronized already. One of the others with a deeper knowledge of the architecture might have some insight there.
--
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:
users@infra.apache.org
[GitHub] [pulsar] devinbost commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828251566
After thinking about this, I noticed a few things.
1. `readMoreEntries()` is recursive, but synchronized locks in Java are reentrant, so synchronizing the method won't create an immediate deadlock
2. as noted, `readMoreEntries()` is called in more than one location by a synchronized method, so a lock is already being placed. If those calls occur from different threads, we will introduce lock contention, which will have a performance impact. Also, since synchronized on a method is equivalent to `synchronized(this)`, we could be placing a larger lock than necessary. In that case, local locks will be less expensive.
3. We want to be mindful of the impact on latency because message dispatch is blocked by this path. However, unless the impact is huge, we should be okay because message consumption will be buffered by permits. So, except for the initial dispatch (where the client doesn't already have messages in its queue), most applications won't be affected by an increase in latency here.
4. It could be argued that many local locks consume more memory than using a single lock, but there are ways of mitigating the memory impact (such as by using `AtomicIntegerFieldUpdater`).
So, I think it becomes a question of latency vs memory impact and whether more local locks should be used instead.
--
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:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828148900
@eolivelli @devinbost @merlimat @codelipenghui @rdhabalia Please review this change that could be a solution to some concurrency issues in PersistentDispatcherMultipleConsumers and PersistentStreamingDispatcherMultipleConsumers classes.
--
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:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828175584
/pulsarbot run-failure-checks
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-830569486
@devinbost this patch did not apply. See the related patch for branch 2.7
--
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:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413#issuecomment-828178351
/pulsarbot run-failure-checks
--
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:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #10413: [Broker] Make Persistent*DispatcherMultipleConsumers.readMoreEntries synchronized
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #10413:
URL: https://github.com/apache/pulsar/pull/10413
--
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:
users@infra.apache.org