[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3835: ARTEMIS-3541 small improvements and some javadoc into instantAutoDeleteAddress

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

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



##########
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 = "forceAutoDeleteAddress", desc = "Automatically delete the address if this was the last queue") boolean autoDeleteAddress) throws Exception;
+                     @Parameter(name = "instantAutoDeleteAddress", desc = "Immediately delete the auto-created address if this was the last queue, ignoring any settings from address-settings") boolean autoDeleteAddress) throws Exception;

Review comment:
       ```suggestion
                        @Parameter(name = "instantAutoDeleteAddress", desc = "Try to auto-delete the address now if it was auto-created, this was the last queue bound to it, and it has no auto-delete delay configured") boolean instantAutoDeleteAddress) throws Exception;
   ```
   The change made the unclear behaviour better in some ways (noting it applied to auto-created addresses), but makes it worse in others. It says it ignores any settings from address-settings, but it doesnt entirely. 
   
   It still adheres to the AutoDeleteAddressesDelay value, which is quite important since if there is one, it will mean the address is _never_ removed if autoDeleteAddress is false for the address-settings and so the reaper doesnt auto-delete it later either.
   
   The suggestion updates the text to cover that, and fixes the arg name.

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -2337,16 +2337,23 @@ public void destroyQueue(final SimpleString queueName,
                             final SecurityAuth session,
                             final boolean checkConsumerCount,
                             final boolean removeConsumers,
-                            final boolean forceAutoDeleteAddress) throws Exception {
-      destroyQueue(queueName, session, checkConsumerCount, removeConsumers, forceAutoDeleteAddress, false);
+                            final boolean instantAutoDeleteAddress) throws Exception {
+      destroyQueue(queueName, session, checkConsumerCount, removeConsumers, instantAutoDeleteAddress, false);
    }
 
+   /**
+    * instantAutoDeleteAddress will immediately remove any auto-created address when the last queue for that address is removed.
+    * Normally the reaper should be responsible for dealing with that but if this parameter is set it will be removed instantly.

Review comment:
       As earlier comment, somewhat overstates the immediacy. It wont be removed if there is a auto delete delay configured. It will then never be removed if the reaper isnt active for the address (as in case that caused the report).
   
   This should really be javadoc rather than just a comment, and be on the interface rather than the impl.




-- 
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