315157973 commented on a change in pull request #8207:
URL: https://github.com/apache/pulsar/pull/8207#discussion_r501132310
##########
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -677,19 +677,16 @@ protected void notifyPendingBatchReceivedCallBack() {
if (opBatchReceive == null || opBatchReceive.future == null) {
return;
}
- notifyPendingBatchReceivedCallBack(opBatchReceive);
+ synchronized (incomingMessages) {
Review comment:
> `incomingMessages` is already a concurrent data structure and it does
not need this `synchronized` block
>
> Also if you use `synchronized` once you must guard it always with this
facility, otherwise you will fall into an inconsistent synchronization.
> I don't know very well this code but it looks like there is no other
instance of `synchronized` over `incomingMessages`
Thanks.
I use `synchronized (incomingMessages)`, just to allow synchronized to have
a final object that can be locked, because I see no other place to use this
object to compete for lock.
I reflected on it, this code can be very confusing, it is better to use
`ReentrantLock`.
----------------------------------------------------------------
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:
[email protected]
