tabish121 commented on code in PR #5738:
URL: https://github.com/apache/activemq-artemis/pull/5738#discussion_r2124870693
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/BridgeImpl.java:
##########
@@ -570,12 +573,17 @@ public HandleStatus handle(final MessageReference ref)
throws Exception {
if (RefCountMessage.isRefTraceEnabled() && ref.getMessage() instanceof
RefCountMessage) {
RefCountMessage.deferredDebug(ref.getMessage(), "Going through the
bridge");
}
+
+ Exception exception = null;
+ HandleStatus status = null;
Review Comment:
This status value could be final which eliminates the gymnastics done later
to capture the result
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/BridgeImpl.java:
##########
@@ -570,12 +573,17 @@ public HandleStatus handle(final MessageReference ref)
throws Exception {
if (RefCountMessage.isRefTraceEnabled() && ref.getMessage() instanceof
RefCountMessage) {
RefCountMessage.deferredDebug(ref.getMessage(), "Going through the
bridge");
}
+
+ Exception exception = null;
Review Comment:
This exception var is not needed, see comment below.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/BridgeImpl.java:
##########
@@ -634,16 +641,26 @@ public HandleStatus handle(final MessageReference ref)
throws Exception {
}
if (server.hasBrokerBridgePlugins()) {
- server.callBrokerBridgePlugins(plugin ->
plugin.afterDeliverBridge(this, ref, status));
+ final HandleStatus finalStatus = status;
+ server.callBrokerBridgePlugins(plugin ->
plugin.afterDeliverBridge(this, ref, finalStatus));
+ return finalStatus;
}
return status;
} catch (Exception e) {
// If an exception happened, we must count down immediately
pendingAcks.countDown();
- throw e;
+ exception = e;
+ }
+
+ } finally {
+ bridgeLock.unlock();
+ if (exception != null) {
Review Comment:
This is wholly unneeded as a finally block will run regardless of an
exception so the local is assured to always be unlocked.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/cluster/impl/BridgeImpl.java:
##########
@@ -634,16 +641,26 @@ public HandleStatus handle(final MessageReference ref)
throws Exception {
}
if (server.hasBrokerBridgePlugins()) {
- server.callBrokerBridgePlugins(plugin ->
plugin.afterDeliverBridge(this, ref, status));
+ final HandleStatus finalStatus = status;
+ server.callBrokerBridgePlugins(plugin ->
plugin.afterDeliverBridge(this, ref, finalStatus));
+ return finalStatus;
}
return status;
} catch (Exception e) {
// If an exception happened, we must count down immediately
pendingAcks.countDown();
- throw e;
+ exception = e;
+ }
+
+ } finally {
+ bridgeLock.unlock();
+ if (exception != null) {
+ throw exception;
}
}
+
+ return status;
Review Comment:
This could be removed after addressing above comments
--
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
