gemmellr commented on code in PR #5484:
URL: https://github.com/apache/activemq-artemis/pull/5484#discussion_r1941199702
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueFactoryImpl.java:
##########
@@ -79,9 +79,9 @@ public void setPostOffice(final PostOffice postOffice) {
public Queue createQueueWith(final QueueConfig config) {
final Queue queue;
if (lastValueKey(config) != null) {
- queue = new LastValueQueue(config.id(), config.address(),
config.name(), config.filter(), config.getPagingStore(),
config.pageSubscription(), config.user(), config.isDurable(),
config.isTemporary(), config.isAutoCreated(), config.deliveryMode(),
config.maxConsumers(), config.isExclusive(), config.isGroupRebalance(),
config.getGroupBuckets(), config.getGroupFirstKey(),
config.consumersBeforeDispatch(), config.delayBeforeDispatch(),
config.isPurgeOnNoConsumers(), lastValueKey(config), config.isNonDestructive(),
config.isAutoDelete(), config.getAutoDeleteDelay(),
config.getAutoDeleteMessageCount(), config.isConfigurationManaged(),
scheduledExecutor, postOffice, storageManager, addressSettingsRepository,
executorFactory.getExecutor(), server, this);
+ queue = new
LastValueQueue(QueueConfiguration.of(config.name()).setAddress(config.address()).setId(config.id()).setUser(config.user()).setDurable(config.isDurable()).setTemporary(config.isTemporary()).setAutoCreated(config.isAutoCreated()).setRoutingType(config.deliveryMode()).setMaxConsumers(config.maxConsumers()).setExclusive(config.isExclusive()).setGroupRebalance(config.isGroupRebalance()).setGroupBuckets(config.getGroupBuckets()).setGroupFirstKey(config.getGroupFirstKey()).setConsumersBeforeDispatch(config.consumersBeforeDispatch()).setDelayBeforeDispatch(config.delayBeforeDispatch()).setPurgeOnNoConsumers(config.isPurgeOnNoConsumers()).setLastValueKey(lastValueKey(config)).setNonDestructive(config.isNonDestructive()).setAutoDelete(config.isAutoDelete()).setAutoDeleteDelay(config.getAutoDeleteDelay()).setAutoDeleteMessageCount(config.getAutoDeleteMessageCount()).setConfigurationManaged(config.isConfigurationManaged()),
config.filter(), config.getPagingStore(), config.p
ageSubscription(), scheduledExecutor, postOffice, storageManager,
addressSettingsRepository, executorFactory.getExecutor(), server, this);
Review Comment:
Since almost all of the options appear to be common, I think it would be
both more readable and more maintainable if a single set of options was
prepared first with all the common config, and then only the difference applied
to them before creating the queue objects and passing them.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueFactoryImpl.java:
##########
@@ -120,9 +120,9 @@ public Queue createQueue(final long persistenceID,
Queue queue;
if (lastValueKey(addressSettings) != null) {
- queue = new LastValueQueue(persistenceID, address, name, filter,
pageSubscription == null ? null : pageSubscription.getPagingStore(),
pageSubscription, user, durable, temporary, autoCreated,
ActiveMQDefaultConfiguration.getDefaultRoutingType(),
ActiveMQDefaultConfiguration.getDefaultMaxQueueConsumers(),
ActiveMQDefaultConfiguration.getDefaultExclusive(),
ActiveMQDefaultConfiguration.getDefaultGroupRebalance(),
ActiveMQDefaultConfiguration.getDefaultGroupBuckets(),
ActiveMQDefaultConfiguration.getDefaultGroupFirstKey(),
ActiveMQDefaultConfiguration.getDefaultConsumersBeforeDispatch(),
ActiveMQDefaultConfiguration.getDefaultDelayBeforeDispatch(),
ActiveMQDefaultConfiguration.getDefaultPurgeOnNoConsumers(),
lastValueKey(addressSettings),
ActiveMQDefaultConfiguration.getDefaultNonDestructive(),
ActiveMQDefaultConfiguration.getDefaultQueueAutoDelete(autoCreated),
ActiveMQDefaultConfiguration.getDefaultQueueAutoDeleteDelay(),
ActiveMQDefaultConfiguration.getDefaultQueueAutoDelete
MessageCount(), false, scheduledExecutor, postOffice, storageManager,
addressSettingsRepository, executorFactory.getExecutor(), server, this);
+ queue = new
LastValueQueue(QueueConfiguration.of(name).setAddress(address).setId(persistenceID).setUser(user).setDurable(durable).setTemporary(temporary).setAutoCreated(autoCreated).setRoutingType(ActiveMQDefaultConfiguration.getDefaultRoutingType()).setConfigurationManaged(false).setMaxConsumers(ActiveMQDefaultConfiguration.getDefaultMaxQueueConsumers()).setExclusive(ActiveMQDefaultConfiguration.getDefaultExclusive()).setGroupRebalance(ActiveMQDefaultConfiguration.getDefaultGroupRebalance()).setGroupBuckets(ActiveMQDefaultConfiguration.getDefaultGroupBuckets()).setGroupFirstKey(ActiveMQDefaultConfiguration.getDefaultGroupFirstKey()).setConsumersBeforeDispatch(ActiveMQDefaultConfiguration.getDefaultConsumersBeforeDispatch()).setDelayBeforeDispatch(ActiveMQDefaultConfiguration.getDefaultDelayBeforeDispatch()).setPurgeOnNoConsumers(ActiveMQDefaultConfiguration.getDefaultPurgeOnNoConsumers()).setLastValueKey(lastValueKey(addressSettings)).setNonDestructive(ActiveMQDefaultConfi
guration.getDefaultNonDestructive()).setAutoDelete(ActiveMQDefaultConfiguration.getDefaultQueueAutoDelete(autoCreated)).setAutoDeleteDelay(ActiveMQDefaultConfiguration.getDefaultQueueAutoDeleteDelay()).setAutoDeleteMessageCount(ActiveMQDefaultConfiguration.getDefaultQueueAutoDeleteMessageCount()),
filter, pageSubscription == null ? null : pageSubscription.getPagingStore(),
pageSubscription, scheduledExecutor, postOffice, storageManager,
addressSettingsRepository, executorFactory.getExecutor(), server, this);
Review Comment:
It seems more than a little weird the LVQ needs all those default configs
specified in the options, but the non-LVQ case below does not, given the LVQ is
a subclass of the non-LVQ, and its _QueueConfiguration_-based constructor
seemingly only calls the superclass then differs only in capture of the LVQ key
value.
Seems like there must be significant room for simplification/consolidation
here (either in the queues, or the options here), rather than this arguably
making things even more complicated/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: 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
