gemmellr commented on code in PR #4941:
URL: https://github.com/apache/activemq-artemis/pull/4941#discussion_r1611299971
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/AMQPBrokerConnection.java:
##########
@@ -701,7 +745,11 @@ private void connectSender(Queue queue,
sender.setSenderSettleMode(SenderSettleMode.UNSETTLED);
sender.setReceiverSettleMode(ReceiverSettleMode.FIRST);
Target target = new Target();
- target.setAddress(targetName);
+ if (targetAddress != null && !targetAddress.isBlank()) {
+ target.setAddress(targetAddress);
Review Comment:
This targetName / targetAddress distinction is a bit awkard with both around
given that 'target address' is usually the normal terminology and what the
value actually sets. Perhaps change the prior one to targetAddress instead of
targetName, then use targetAddressOverride for the new 'use this instead' value
to clarify things?
Similarly for the receiver bit above.
EDIT: or see other comment, simplify this and move decision to the call site
where config is being pulled originally.
##########
artemis-server/src/main/resources/schema/artemis-configuration.xsd:
##########
@@ -2228,6 +2228,50 @@
</xsd:attribute>
</xsd:complexType>
+ <xsd:complexType name="amqp-broker-connect-sender-type">
+ <xsd:complexContent>
+ <xsd:extension base="amqp-address-match-type">
+ <xsd:attribute name="target-address" type="xsd:string"
use="optional">
+ <xsd:annotation>
+ <xsd:documentation>
+ This address to assign to the AMQP sender link Target.
Review Comment:
This -> The? Similarly below.
Worth clarifying that this overrides the normal use of the queues associated
address as the target? Also that you probably dont typically want to use this
with a 'match' style configuration as every matched queue's associated sender
will then all send to the same target address? Should that config be prevented?
Maybe need some documentation around all this either way?
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/AMQPBrokerConnection.java:
##########
@@ -282,18 +285,52 @@ public void validateMatching(Queue queue,
AMQPBrokerConnectionElement connection
public void createLink(Queue queue, AMQPBrokerConnectionElement
connectionElement) {
if (connectionElement.getType() == AMQPBrokerConnectionAddressType.PEER)
{
Symbol[] dispatchCapability = new
Symbol[]{AMQPMirrorControllerSource.QPID_DISPATCH_WAYPOINT_CAPABILITY};
- connectSender(queue, queue.getAddress().toString(), null, null, null,
null, dispatchCapability, null);
- connectReceiver(protonRemotingConnection, session, sessionContext,
queue, dispatchCapability);
- } else {
- if (connectionElement.getType() ==
AMQPBrokerConnectionAddressType.SENDER) {
- connectSender(queue, queue.getAddress().toString(), null, null,
null, null, null, null);
+ connectSender(queue, queue.getAddress().toString(), null, null, null,
null, null, null, dispatchCapability);
+ connectReceiver(protonRemotingConnection, session, sessionContext,
queue, null, dispatchCapability);
+ } else if (connectionElement.getType() ==
AMQPBrokerConnectionAddressType.SENDER) {
+ String targetAddress = null;
+ Symbol[] targetCapabilities = null;
+
+ if (connectionElement instanceof AMQPSenderBrokerConnectionElement) {
+ final AMQPSenderBrokerConnectionElement sender =
(AMQPSenderBrokerConnectionElement) connectionElement;
+
+ targetAddress = sender.getTargetAddress();
+ targetCapabilities =
parseTerminusCapabilities(sender.getTargetCapabilities());
}
- if (connectionElement.getType() ==
AMQPBrokerConnectionAddressType.RECEIVER) {
- connectReceiver(protonRemotingConnection, session, sessionContext,
queue);
+
+ connectSender(queue, queue.getAddress().toString(), null, null, null,
null, null, targetAddress, targetCapabilities);
Review Comment:
Could we actually just move the other 'is an explicit override address
defined' bit here, and then simply pass 'targetName' as before (perhaps renamed
to targetAddress though?), but which is then either the queues address as
originally, or the new configured 'override' target if defined? Would save
passing around 2 competing values for 1 thing and leave the other code simpler
/ as-is.
Similarly for the receiver bit below.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
