[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3833: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3833:
URL: https://github.com/apache/activemq-artemis/pull/3833#discussion_r741347945



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueManagerImpl.java
##########
@@ -72,7 +72,7 @@ public static void performAutoDeleteQueue(ActiveMQServer server, Queue queue) {
       ActiveMQServerLogger.LOGGER.autoRemoveQueue("" + queue.getName(), queue.getID(), "" + queue.getAddress());
 
       try {
-         server.destroyQueue(queueName, null, true, false, settings.isAutoDeleteAddresses(), true);
+         server.destroyQueue(queueName, null, true, false, false, true);

Review comment:
       This almost certainly rendered the settings unused, so they should have been removed as well.

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java
##########
@@ -672,13 +672,13 @@ void destroyQueue(SimpleString queueName,
                      SecurityAuth session,
                      boolean checkConsumerCount,
                      boolean removeConsumers,
-                     boolean autoDeleteAddress) throws Exception;
+                     boolean forceAutoDeleteAddress) throws Exception;
 
    void destroyQueue(SimpleString queueName,
                      SecurityAuth session,
                      boolean checkConsumerCount,
                      boolean removeConsumers,
-                     boolean autoDeleteAddress,
+                     boolean forceAutoDeleteAddress,

Review comment:
       Could use some doc here too to explain what it does.

##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java
##########
@@ -1090,7 +1090,7 @@ void destroyQueue(@Parameter(name = "name", desc = "Name of the queue to destroy
    @Operation(desc = "Destroy a queue", impact = MBeanOperationInfo.ACTION)
    void destroyQueue(@Parameter(name = "name", desc = "Name of the queue to destroy") String name,
                      @Parameter(name = "removeConsumers", desc = "Remove consumers of this queue") boolean removeConsumers,
-                     @Parameter(name = "autoDeleteAddress", desc = "Automatically delete the address if this was the last queue") boolean autoDeleteAddress) throws Exception;
+                     @Parameter(name = "forceAutoDeleteAddress", desc = "Automatically delete the address if this was the last queue") boolean autoDeleteAddress) throws Exception;

Review comment:
       This name could be fairly misleading with that doc along with it, which isnt precisely what it does. I would consider it reasonable to think think a 'force' auto deletion described that way would always delete and do it there and then, which it doesnt necessarily. It only deletes due to this if the address was itself auto-created, and only if it has an exactly-0 autoDeleteDelay.
   
   This would again give super-surprising behaviour if auto-delete-delay is also disabled in the broker config generally, as it was leading to finding this issue, since the address would then never be removed (by the reaper), even despite this requested 'forcing'.
   




-- 
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-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org