lhotari commented on code in PR #25990:
URL: https://github.com/apache/pulsar/pull/25990#discussion_r3388633194
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -1372,13 +1372,12 @@ protected boolean isNormalReadAllowed() {
}
-
- protected synchronized boolean shouldPauseDeliveryForDelayTracker() {
- return delayedDeliveryTracker.isPresent() &&
delayedDeliveryTracker.get().shouldPauseAllDeliveries();
+ protected boolean shouldPauseDeliveryForDelayTracker() {
+ return
delayedDeliveryTracker.map(DelayedDeliveryTracker::shouldPauseAllDeliveries).orElse(false);
}
@Override
- public synchronized long getNumberOfDelayedMessages() {
+ public long getNumberOfDelayedMessages() {
Review Comment:
this is fine
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -1464,20 +1463,15 @@ public PersistentTopic getTopic() {
}
- public synchronized long getDelayedTrackerMemoryUsage() {
+ public long getDelayedTrackerMemoryUsage() {
Review Comment:
the implementation of getBufferMemoryUsage isn't thread safe in
BucketDelayedDeliveryTracker or in InMemoryDelayedDeliveryTracker.
One particular detail about RoaringBitmaps is that it's not thread safe for
even plain concurrent reads and data races aren't the only problem. Reads can
mutate RoaringBitmap internal state. I learned that quite recently and there
are other bugs in Pulsar impacted by this too since StampedLock/ReadWriteLocks
aren't safe with RoaringBitmap methods. There has been changes in the project
to prevent internal datastructure corruption caused by concurrent reads, but I
don't think that it has been fully resolved. I checked one of the recent
changes and it looks invalid although it was merged to the project. Making a
single method synchronized is useless if reads or writes happen outside of the
same object monitor.
Thread safety of RoaringBitmap is documented here:
https://github.com/RoaringBitmap/RoaringBitmap?tab=readme-ov-file#thread-safety
(there is no commitment for thread safety).
Since BucketDelayedDeliveryTracker heavily uses RoaringBitmaps, I think that
we need a different solution to avoid blocking when stats are requested. One
possibility would be to have a background thread refresh stats after there has
been activity since the last operation. The instances would have to be
immutable for this to work properly.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -1464,20 +1463,15 @@ public PersistentTopic getTopic() {
}
- public synchronized long getDelayedTrackerMemoryUsage() {
+ public long getDelayedTrackerMemoryUsage() {
return
delayedDeliveryTracker.map(DelayedDeliveryTracker::getBufferMemoryUsage).orElse(0L);
}
- public synchronized Map<String, TopicMetricBean>
getBucketDelayedIndexStats() {
- if (delayedDeliveryTracker.isEmpty()) {
- return Collections.emptyMap();
- }
-
- if (delayedDeliveryTracker.get() instanceof
BucketDelayedDeliveryTracker) {
- return ((BucketDelayedDeliveryTracker)
delayedDeliveryTracker.get()).genTopicMetricMap();
- }
-
- return Collections.emptyMap();
+ public Map<String, TopicMetricBean> getBucketDelayedIndexStats() {
+ return delayedDeliveryTracker
+ .filter(BucketDelayedDeliveryTracker.class::isInstance)
+ .map(tracker -> ((BucketDelayedDeliveryTracker)
tracker).genTopicMetricMap())
Review Comment:
BucketDelayedDeliveryTracker.genTopicMetricMap is not thread safe. It should
be made thread safe.
--
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]
