tabish121 commented on code in PR #5760:
URL: https://github.com/apache/activemq-artemis/pull/5760#discussion_r2142987459
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -307,14 +315,28 @@ public int handleDeliver(MessageReference reference,
ICoreMessage message) {
reference.setProtocolData(MessageId.class,
dispatch.getMessage().getMessageId());
session.deliverMessage(dispatch);
// Prevent races with other updates that can lead to credit going
negative and starving consumers.
- currentWindow.updateAndGet(i -> i > 0 ? i - 1 : i);
+ currentWindow.updateAndGet(AMQConsumer::decrementWindow);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Decrementing credit, current={}",
currentWindow.get());
Review Comment:
This log says decrementing but it already decremented so it should either be
moved up, or reworded to indicate its reporting the post op result
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -413,6 +453,9 @@ public boolean hasCredits() {
public void processMessagePull(MessagePull messagePull) throws Exception {
currentWindow.incrementAndGet();
+ if (logger.isDebugEnabled()) {
+ logger.debug("Incrementing credit for processMessagePull, current =
{}", currentWindow.get());
Review Comment:
It was already incremented, so the value reported is post change, where the
current log makes it seems as if its reports the pre-change value
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -438,6 +493,9 @@ public org.apache.activemq.command.ActiveMQDestination
getOpenwireDestination()
public void setPrefetchSize(int prefetchSize) {
this.prefetchSize = prefetchSize;
this.currentWindow.set(prefetchSize);
+ if (logger.isTraceEnabled()) {
+ logger.trace("Updating credits with a new prefetchSize",
prefetchSize, new Exception("trace"));
Review Comment:
Same as above, the log message is a bit misleading
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumer.java:
##########
@@ -307,14 +315,28 @@ public int handleDeliver(MessageReference reference,
ICoreMessage message) {
reference.setProtocolData(MessageId.class,
dispatch.getMessage().getMessageId());
session.deliverMessage(dispatch);
// Prevent races with other updates that can lead to credit going
negative and starving consumers.
- currentWindow.updateAndGet(i -> i > 0 ? i - 1 : i);
+ currentWindow.updateAndGet(AMQConsumer::decrementWindow);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Decrementing credit, current={}",
currentWindow.get());
+ }
return size;
} catch (Throwable t) {
logger.warn("Error during message dispatch", t);
return 0;
}
}
+ static int decrementWindow(int value) {
+ if (value > 0) {
+ return value - 1;
+ } else {
+ if (logger.isDebugEnabled()) {
+ logger.debug("preventing a negative value={}", value, new
Exception("trace"));
Review Comment:
Would be nice if the log was a tad more descriptive, "preventing the credit
window from going negative" or some such just to illuminate the future reader.
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
