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.
##########
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:
Fine with me, as I said in the other PR I think that was a likely
interpretation of what this should do, which is why the description was an
issue since it didn't really describe what it actually did.
The autoDeleteAddress arg still needs renamed.
##########
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:
Or maybe it doesnt. I would be fine leave them all as autoDeleteAddress
rather than renaming any of them.
##########
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:
Or maybe it doesnt. I would be fine leaving them all as
autoDeleteAddress rather than renaming any of them.
##########
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:
Or maybe it doesnt. I would be fine leaving them all as
autoDeleteAddress rather than renaming any of them (from their original state
before the other PR).
##########
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.
##########
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:
Fine with me, as I said in the other PR I think that was a likely
interpretation of what this should do, which is why the description was an
issue since it didn't really describe what it actually did.
The autoDeleteAddress arg still needs renamed.
##########
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:
Or maybe it doesnt. I would be fine leave them all as autoDeleteAddress
rather than renaming any of them.
##########
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:
Or maybe it doesnt. I would be fine leaving them all as
autoDeleteAddress rather than renaming any of them.
##########
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:
Or maybe it doesnt. I would be fine leaving them all as
autoDeleteAddress rather than renaming any of them (from their original state
before the other PR).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
