chamons commented on code in PR #25681:
URL: https://github.com/apache/pulsar/pull/25681#discussion_r3209267306
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java:
##########
@@ -122,21 +122,29 @@ private static long trimLowerBit(long timestamp, int
bits) {
}
@Override
- public boolean addMessage(long ledgerId, long entryId, long deliverAt) {
+ public synchronized boolean addMessage(long ledgerId, long entryId, long
deliverAt) {
Review Comment:
If I [remove the synchronized in that
file](https://gist.github.com/chamons/954c0d442ecee83817868e4c1d838704)
My new test (org.apache.pulsar.broker.delayed.InMemoryDeliveryTrackerTest)
fails with [multiple different
exceptions](https://gist.github.com/chamons/2bf7e73f9b72901c3db9958e7940b775).
```
java.lang.AssertionError: No exceptions should occur during concurrent
operations expected [0] but found [5]
```
The test itself seemed reasonable.
I might have added too many, I went with the heuristic of "delayedMessageMap
and delayedMessagesCount must be kept in sync, so any method that uses them
should hold the lock". Close is the most obvious case where that might be
overkill, I'm unsure.
--
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]
