gemmellr commented on code in PR #5220:
URL: https://github.com/apache/activemq-artemis/pull/5220#discussion_r1777398695
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/mirror/NoForwardMirrorController.java:
##########
@@ -0,0 +1,5 @@
+package org.apache.activemq.artemis.core.server.mirror;
Review Comment:
Needs the license header added
##########
artemis-server/src/main/resources/schema/artemis-configuration.xsd:
##########
@@ -2306,6 +2306,14 @@
</xsd:documentation>
</xsd:annotation>
</xsd:attribute>
+ <xsd:attribute name="no-forwarding" type="xsd:boolean" use="optional"
default="false">
+ <xsd:annotation>
+ <xsd:documentation>
+ If this is true, the mirror at the opposite end of the link
will not forward data coming from that link to any other mirrors down the line.
Review Comment:
"the mirror at the opposite end of the link will not forward data coming
from that link..." might be clearer as something like "the broker at the
opposite end of the mirror link will not forward messages or instructions
coming from this broker..."
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/mirror/NoForwardMirrorController.java:
##########
@@ -0,0 +1,5 @@
+package org.apache.activemq.artemis.core.server.mirror;
+
+public interface NoForwardMirrorController extends MirrorController{
Review Comment:
I would go with e.g MirrorControllerTarget or TargetMirrorController, and
then only the AMQPMirrorControllerTarget (and the substitute inside AckManager)
would implement it, not the AMQPMirrorControllerSource. We can use the same
interface for other Target-specific detail later without renaming it or adding
others, and more easily follow target-specific functionality around the broker.
In the usage we only care if the message is being operated on via a mirror
target, by a mirror source. The mirror source doesnt need to implement this
method.
EDIT: later bit already fixed.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/mirror/AMQPMirrorControllerSource.java:
##########
@@ -236,6 +238,10 @@ public void deleteAddress(AddressInfo addressInfo) throws
Exception {
if (ignoreAddress(addressInfo.getName())) {
return;
}
+ if (isBlockedByNoForward()) {
+ return;
+ }
+
Review Comment:
I would do this earlier; no point checking the specific address is permitted
(the bit above) if we aren't allowed to forward it _anywhere_.
Same for other bits futher down
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/amqpBrokerConnectivity/AMQPMirrorBrokerConnectionElement.java:
##########
@@ -75,6 +77,15 @@ public AMQPMirrorBrokerConnectionElement
setQueueCreation(boolean queueCreation)
return this;
}
+ public boolean getNoForward() {
+ return noForward;
+ }
+
+ public AMQPMirrorBrokerConnectionElement setNoForward(boolean noForward) {
Review Comment:
The naming here influences the namign needed for Broker Properties
.properties / .yaml based configs. Here the property name is "NoForward", but
in the xml config the wording "no-forwarding". They should probably be
consistent in the wording i.e the _forward_, or _forwarding_) if there isn't a
significant reason for them to differ.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/mirror/AMQPMirrorControllerSource.java:
##########
@@ -559,6 +587,10 @@ public void preAcknowledge(final Transaction tx, final
MessageReference ref, fin
return;
}
+ if (isBlockedByNoForward()) {
+ return;
+ }
+
Review Comment:
I suspect a different check will be needed here, inspecting the specific
message; this is ultimately called by the queue, which I expect usually wont
have a mirror target controller set. It will also want to accommodate not
sending acks for no-forward messages [that it didnt send to begin with] even
across broker restarts, due to expiry, etc etc.
I expect the test-peer based testing I know you are working on will
demonstrate this.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/mirror/NoForwardMirrorController.java:
##########
@@ -0,0 +1,5 @@
+package org.apache.activemq.artemis.core.server.mirror;
+
+public interface NoForwardMirrorController extends MirrorController{
+ public boolean getNoForward();
Review Comment:
As a boolean, isNoForward would be more typical.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/mirror/AMQPMirrorControllerSource.java:
##########
@@ -60,7 +61,7 @@
import static
org.apache.activemq.artemis.protocol.amqp.connect.mirror.AMQPMirrorControllerTarget.getControllerInUse;
-public class AMQPMirrorControllerSource extends BasicMirrorController<Sender>
implements MirrorController, ActiveMQComponent {
+public class AMQPMirrorControllerSource extends BasicMirrorController<Sender>
implements ActiveMQComponent, NoForwardMirrorController {
Review Comment:
Per other comments, feels like this should still just implement the base
MirrorController interface only?
--
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
