Skip to content

[ARTEMIS-1241] check for FQQN and autocreate queue using the configur…#1759

Closed
gtully wants to merge 1 commit into
apache:masterfrom
gtully:ARTEMIS-1241
Closed

[ARTEMIS-1241] check for FQQN and autocreate queue using the configur…#1759
gtully wants to merge 1 commit into
apache:masterfrom
gtully:ARTEMIS-1241

Conversation

@gtully
Copy link
Copy Markdown
Contributor

@gtully gtully commented Jan 9, 2018

…ed default routing type

@gtully
Copy link
Copy Markdown
Contributor Author

gtully commented Jan 9, 2018

I added to some existing auto create address logic in the code path which suffices for my test case. It does appear a little hacky to have three xxToUse local variables and the toString messing on simpleString does not look right.
One thought was to pass a null routingtype to this method but it is not clear to me where that decision should be made. Pulling the value from config for the auto creation case does make sense however.
Please review and maybe it is time to consider pulling all of the auto creation logic into one place.

@jbertram
Copy link
Copy Markdown
Contributor

jbertram commented Jan 9, 2018

This looks OK to me. @mtaylor, will you review this since you opened the JIRA?

addressToUse = new SimpleString(compositeAddress.getAddress());
queueNameToUse = new SimpleString(compositeAddress.getQueueName());
AddressSettings as = getAddressSettingsRepository().getMatch(addressToUse.toString());
routingTypeToUse = as.getDefaultAddressRoutingType();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The routing type needs to come from the address. If the address doesn't exist, then it should be auto-created before the queue is created.

}

final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(routingType).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).build();
final QueueConfig queueConfig = queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(routingTypeToUse).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).build();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The routing type should be decided by the protocol handler and passed in.

final QueueConfig.Builder queueConfigBuilder;

final SimpleString addressToUse = address == null ? queueName : address;
RoutingType routingTypeToUse = (routingType == null ? ActiveMQDefaultConfiguration.getDefaultRoutingType() : routingType);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic should live in the OpenWire protocol manager.

SimpleString topic = new SimpleString("VirtualTopic.Orders");
SimpleString subscriptionQ = new SimpleString("Consumer.A");

// defaults are false via test setUp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test the case where an address already exists and the default type is ANYCAST.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mtaylor - I see, that getDefaultAddressRoutingType should only be used by auto address creation. The queue routing type needs to be figured out from the address.

@gtully
Copy link
Copy Markdown
Contributor Author

gtully commented Jan 12, 2018

@mtaylor Pushed an update, with the check in the openwire session the change set is smaller and the new tests with an existing address work as expected.
the prefixing remains and the case of multiple routing types is handled already by returning the first entry in the set which may need a revisit when the prefix is handled.

CompositeAddress compositeAddress = CompositeAddress.getQueueName(queueName.toString());
addressToUse = new SimpleString(compositeAddress.getAddress());
queueNameToUse = new SimpleString(compositeAddress.getQueueName());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtully This looks good, except you also need to check that the address exists and if auto-create addresses is switched on. You can get this information from the bindings query.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server.createQueue() will create the address if it doesn't already exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, all the tests had a createProducer earlier that auto created the address.
With a createConsumer first I see the problem, the bindingQuery AddressInfo is null.
Passing a null routing type in that case and some tidy up in auto address creation in serverimpl works but the routingType comes from defaultConfiguration rather than the address settings in that case.
Question - should the caller figure the routingType from the addressSettings and if so should the logic for null routingType in createQueue get whacked?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the logic to figure the routingType as the path of least resistance and the most explicit. The null routing type autocreateaddress case does not work in createQueue - maybe that is an indication that the relevant code can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants