gemmellr commented on code in PR #4421:
URL: https://github.com/apache/activemq-artemis/pull/4421#discussion_r1162547033
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java:
##########
@@ -301,6 +338,7 @@ public QueueConfiguration setName(SimpleString name) {
if (CompositeAddress.isFullyQualified(name)) {
this.name = CompositeAddress.extractQueueName(name);
this.address = CompositeAddress.extractAddressName(name);
+ this.fqqn = true;
Review Comment:
setAddress seems like it should have a similar update since it also does the
isFullyQualified check and can change the name and address values like this
method. (This also points to some missing unit tests that should be added).
Should probably use Boolean.TRUE given the field is Boolean (EDIT: or change
field to boolean as in next comment).
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java:
##########
@@ -607,6 +645,16 @@ public QueueConfiguration setAutoCreated(Boolean
autoCreated) {
return this;
}
+ /**
+ * Based on if the name or address uses FQQN when set
+ *
+ * defaults to {@code false}
+ * @return
+ */
+ public Boolean isFqqn() {
+ return fqqn == null ? false : fqqn;
Review Comment:
The return type is Boolean so it should probably use Boolean.FALSE for the
return, but given it goes to the effort of trying not to return null anyway,
changing the field and return type to primitive boolean would probably be nicer.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java:
##########
@@ -174,7 +174,7 @@ private RoutingType getRoutingType(Symbol[] symbols,
SimpleString address) {
}
RoutingType defaultRoutingType =
sessionSPI.getDefaultRoutingType(address);
defaultRoutingType = defaultRoutingType == null ?
ActiveMQDefaultConfiguration.getDefaultRoutingType() : defaultRoutingType;
- return defaultRoutingType;
+ return sessionSPI.getRoutingTypeFromPrefix(address, defaultRoutingType);
Review Comment:
This is changing the behaviour for a producer, but the consumer side hasnt
changed. If we did change this, then I'd expect there should be analogous
changes being made in ProtonServerSenderContext or the producer and consumer
side behaviour could become inconsistent \[again, as they were until recent
changes from Tim (e.g
https://github.com/apache/activemq-artemis/commit/ca66028b2a596a4ab5a61ec57633fed3e7b85b22)
\].
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/BrokerDefinedMulticastConsumerTest.java:
##########
@@ -65,8 +75,6 @@ public void testConsumeFromSingleQueueOnAddressSameName()
throws Exception {
public void testConsumeWhenOnlyAnycast() throws Exception {
server.addAddressInfo(new AddressInfo(address, RoutingType.ANYCAST));
- sendMessages(address.toString(), 1);
-
Review Comment:
Is this removed due to disabling the autocreation? Creating the anycast
queue if needed might be the better change than removing the send. I believe it
was probably there to show attaching to the address worked, before the later
expected failure when attaching the consumer to the same address, due to
explicitly requesting a 'topic / multicast' address and finds the anycast
address instead.
##########
artemis-junit/artemis-junit-4/src/test/java/org/apache/activemq/artemis/junit/EmbeddedActiveMQResourceCustomConfigurationTest.java:
##########
@@ -38,13 +38,16 @@ public class
EmbeddedActiveMQResourceCustomConfigurationTest {
static final String TEST_ADDRESS = "test.address";
QueueConfiguration queueConfiguration = new
QueueConfiguration(TEST_QUEUE).setAddress(TEST_ADDRESS);
- Configuration customConfiguration = new
ConfigurationImpl().setPersistenceEnabled(false).setSecurityEnabled(true).addQueueConfiguration(queueConfiguration);
+ Configuration customConfiguration = new
ConfigurationImpl().setPersistenceEnabled(false).setSecurityEnabled(true).addQueueConfiguration(queueConfiguration).addAcceptorConfiguration("amqp",
"tcp://localhost:5672");
Review Comment:
Is this change related? The test itself didnt seem to change, and so doesn't
seem to do anything with AMQP?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpSendReceiveTest.java:
##########
@@ -81,9 +82,9 @@ public void testAcceptWithoutSettling() throws Exception {
AmqpConnection connection = addConnection(client.connect());
AmqpSession session = connection.createSession();
- AmqpReceiver receiver = session.createReceiver(getQueueName());
+ AmqpReceiver receiver = session.createReceiver(getPrefixedQueueName());
Review Comment:
Its not clear to me that this and a lot of the similar changes made should
be needed or are a good idea, potentially implying breakage of a load of
existing applications?
For clients/applications that dont 'request a particular routing type'
(using e.g the terminus capabilities like the JMS client uses, or prefixed
address as artemis also supports), which can be many since AMQP 1.0 itself
doesnt and only uses address names to reference nodes, they should just be able
to operate against an existing anycast or multicast address/queue if it was
already predefined, as it was in this test. It shouldnt need to use a prefixed
address name,and I believe hasnt up until now. For clients/applications that
explicitly do request a type, sure it can/should throw if they instead hit
something different.
--
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]
