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/09 10:12:58 UTC
[GitHub] [pulsar] lhotari opened a new pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits
lhotari opened a new pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180
### Motivation
AtomicIntegerFieldUpdater should be used to update `volatile int` fields.
### Modifications
Use existing `TOTAL_AVAILABLE_PERMITS_UPDATER` to mutate `totalAvailablePermits` field.
--
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 a change in pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#discussion_r621785353
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -184,7 +184,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
messagesToRedeliver.add(ledgerId, entryId);
redeliveryTracker.addIfAbsent(PositionImpl.get(ledgerId, entryId));
});
- totalAvailablePermits -= consumer.getAvailablePermits();
+ TOTAL_AVAILABLE_PERMITS_UPDATER.addAndGet(this, -consumer.getAvailablePermits());
Review comment:
@merlimat I re-opened this PR. `readMoreEntries` method isn't synchronized and it's getting called from this location without synchronization: https://github.com/apache/pulsar/blob/a1cebdb3e7cc3b8ee3ce567058182e7d31c762d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2439-L2451 .
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816738791
/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] lhotari commented on pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816658084
/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] lhotari closed pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-828120292
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
Is there a reason why it's not a synchronized method? 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
Synchronization is missing in this location:
https://github.com/apache/pulsar/blob/a1cebdb3e7cc3b8ee3ce567058182e7d31c762d2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2439-L2451
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-828642579
Not needed since #10413 was merged.
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816656107
btw. This problem was only in the `PersistentDispatcherMultipleConsumers` class. Similar code in `NonPersistentDispatcherMultipleConsumers` didn't have the issue: https://github.com/apache/pulsar/blob/31f831574c3a65bae0e76801facaf2deb0b17fbb/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentDispatcherMultipleConsumers.java#L94-L148
--
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 edited a comment on pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-828642579
This PR is not needed since #10413 was merged.
--
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 closed pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816611708
This PR was co-authored with @devinbost . Thanks for @devinbost for pinpointing the code location around `totalAvailablePermits` in `PersistentDispatcherMultipleConsumers` class. This fix wouldn't have happened without @devinbost 's persistent investigation work. This change might fix issues such as #6054.
@codelipenghui @merlimat @eolivelli @rdhabalia Please review this change.
--
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 a change in pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#discussion_r610772740
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -184,7 +184,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
messagesToRedeliver.add(ledgerId, entryId);
redeliveryTracker.addIfAbsent(PositionImpl.get(ledgerId, entryId));
});
- totalAvailablePermits -= consumer.getAvailablePermits();
+ TOTAL_AVAILABLE_PERMITS_UPDATER.addAndGet(this, -consumer.getAvailablePermits());
Review comment:
yes you are right, it seems that all updates are already happening withing `synchronized` methods.
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816797745
/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] lhotari commented on a change in pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#discussion_r610772740
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -184,7 +184,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
messagesToRedeliver.add(ledgerId, entryId);
redeliveryTracker.addIfAbsent(PositionImpl.get(ledgerId, entryId));
});
- totalAvailablePermits -= consumer.getAvailablePermits();
+ TOTAL_AVAILABLE_PERMITS_UPDATER.addAndGet(this, -consumer.getAvailablePermits());
Review comment:
yes you are right, it seems that all updates are already happening withing `synchronized` methods. It seems that the case is similar in `NonPersistentDispatcherMultipleConsumers` where the `TOTAL_AVAILABLE_PERMITS_UPDATER` is currently used.
--
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 a change in pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#discussion_r610756824
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -175,7 +175,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
log.info("[{}] All consumers removed. Subscription is disconnected", name);
closeFuture.complete(null);
}
- totalAvailablePermits = 0;
+ TOTAL_AVAILABLE_PERMITS_UPDATER.set(this, 0);
Review comment:
true. I tried to make it consistent with the style used in NonPersistentDispatcherMultipleConsumers: https://github.com/apache/pulsar/blob/31f831574c3a65bae0e76801facaf2deb0b17fbb/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentDispatcherMultipleConsumers.java#L104
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-828134630
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
I'm running an experiment to see if tests pass when readEntries method is made `synchronized`.
--
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 commented on a change in pull request #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#discussion_r610736439
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -184,7 +184,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
messagesToRedeliver.add(ledgerId, entryId);
redeliveryTracker.addIfAbsent(PositionImpl.get(ledgerId, entryId));
});
- totalAvailablePermits -= consumer.getAvailablePermits();
+ TOTAL_AVAILABLE_PERMITS_UPDATER.addAndGet(this, -consumer.getAvailablePermits());
Review comment:
The increments/decrement on the `totalAvailablePermits` should already be safe because they're all done while holding the mutex on the dispatcher object.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -175,7 +175,7 @@ public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceE
log.info("[{}] All consumers removed. Subscription is disconnected", name);
closeFuture.complete(null);
}
- totalAvailablePermits = 0;
+ TOTAL_AVAILABLE_PERMITS_UPDATER.set(this, 0);
Review comment:
This would be equivalent to `totalAvailablePermits = 0`
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-816852652
I'm closing this PR since as @merlimat pointed out, all mutations are already safe since they are done in `synchronized` methods in `PersistentDispatcherMultipleConsumers` class.
--
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 #10180: Use AtomicIntegerFieldUpdater to mutate volatile int totalAvailablePermits in PersistentDispatcherMultipleConsumers class
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10180:
URL: https://github.com/apache/pulsar/pull/10180#issuecomment-828148394
I opened a separate PR #10413 to make the `readMoreEntries` method `synchronized`.
--
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