ARTEMIS-5282 remove deprecated queue-related methods #5484
Conversation
| @@ -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.pageSubscription(), scheduledExecutor, postOffice, storageManager, addressSettingsRepository, executorFactory.getExecutor(), server, this); | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
I went back and forth on this for awhile today trying to decide if this code should be updated or just left alone. Ultimately I decided it should be removed along with the other deprecated code. I pushed a new commit with that change plus an updated commit message with an explanation. Let me know what you think.
There was a problem hiding this comment.
I hadn't even noticed that the QueueConfig-using factory method I was commenting on was itself also deprecated (since that doesnt show in the initial diff context of the change, and since the described aim was 'deprecated QueueImpl constructor removal', I simply didnt think to look further at the factory method since it was just being updated to use the new QueueImpl constructors).
Given the factory methods were deprecated in the same commit as the QueueImpl constructors nearly 5 years ago, and you say the factories are also still really internal/impl detail, just removing the deprecated factory methods rather than updating them seems fair.
There was a problem hiding this comment.
Though I ran the full test suite on this and saw some failures that look like theyll be related to the latest changes.
There was a problem hiding this comment.
Full test-suite is green on this with the latest push (fdf1b03).
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueFactoryImpl.java
Outdated
Show resolved
Hide resolved
jbertram
force-pushed
the
ARTEMIS-5282
branch
from
a6a957e to
f59f032
Compare
jbertram
changed the title
jbertram
force-pushed
the
ARTEMIS-5282
branch
2 times, most recently
from
6bf1a5d to
986c9c2
Compare
All but one QueueImpl constructor was deprecated years ago in 2efa44d. Since QueueImpl is an internal implementation object (as the name suggests) these constructors can be completely removed and didn't technically need to be deprecated in the first place. Furthermore, the methods from QueueFactoryImpl which use these constructors were also deprecated in that same commit. Despite the fact that there is a QueueFactory interface these objects are still for internal use/implementation. Therefore, these deprecated methods can and should be removed.
jbertram
force-pushed
the
ARTEMIS-5282
branch
from
986c9c2 to
fdf1b03
Compare
All but one QueueImpl constructor was deprecated years ago in 2efa44d. Since QueueImpl is an internal implementation object (as the name suggests) these constructors can be completely removed and didn't technically need to be deprecated in the first place.
Furthermore, the methods from QueueFactoryImpl which use these constructors were also deprecated in that same commit. Despite the fact that there is a QueueFactory interface these objects are still for internal use/implementation. Therefore, these deprecated methods can and should be removed.