You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/10/27 03:01:10 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
jbertram opened a new pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819
Commit 481b73c8cabf9d7c88bca048b97bd587661b9c52 from ARTEMIS-3502
inadvertently broke this functionality. This commit restores the
original behavior. When autoDeleteAddress is true then the queue's
address should be automatically deleted regardless of whether or not it
is temporary (assuming it meets all the other criteria).
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-956324860
the check on the flag is fine.. it will immediately remove the address only if it's from management.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-956324860
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#discussion_r737814290
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -2397,15 +2397,13 @@ public void destroyQueue(final SimpleString queueName,
callBrokerQueuePlugins(plugin -> plugin.afterDestroyQueue(queue, address, session, checkConsumerCount, removeConsumers, autoDeleteAddress));
}
- if (queue.isTemporary()) {
- AddressInfo addressInfo = getAddressInfo(address);
+ AddressInfo addressInfo = getAddressInfo(address);
- if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
- try {
- removeAddressInfo(address, session);
- } catch (ActiveMQDeleteAddressException e) {
- // Could be thrown if the address has bindings or is not deletable.
- }
+ if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
Review comment:
We should either remove it.. or if we want to keep we would have to flag it somehow the reaper will know to remove it.. (like @gemmellr said)
@jbertram ^
--
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
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#discussion_r737653829
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -2397,15 +2397,13 @@ public void destroyQueue(final SimpleString queueName,
callBrokerQueuePlugins(plugin -> plugin.afterDestroyQueue(queue, address, session, checkConsumerCount, removeConsumers, autoDeleteAddress));
}
- if (queue.isTemporary()) {
- AddressInfo addressInfo = getAddressInfo(address);
+ AddressInfo addressInfo = getAddressInfo(address);
- if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
- try {
- removeAddressInfo(address, session);
- } catch (ActiveMQDeleteAddressException e) {
- // Could be thrown if the address has bindings or is not deletable.
- }
+ if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
Review comment:
> you should instead make sure the AddressReaper would remove such addresses.. I would revert this fix here and would add a check for auto-delete address on the reaper.
Do you mean adding some sort of additional flag that this method would set (if autoDeleteAddress was passed true by management) to indicate the reaper should later delete the address even when it otherwise wouldnt? The reaper is already checking whether it should auto-delete an address, but decides not to for this case since auto-delete-addresses is false in the broker config.
Its not clear to me that it shouldnt just be deleted here, so long as it is a request from management (as earlier, the impl would need adjusted to account for what it is being used for). The queue already has been at this point, and the alternative if there was no autoDeleteAddress boolean on destroyQueue would presumably be to go and perform an explicit deletion on the address afterwards as well using management, which I assume also acts immediately.
--
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
[GitHub] [activemq-artemis] jbertram closed pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819
--
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
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#discussion_r737543838
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -2397,15 +2397,13 @@ public void destroyQueue(final SimpleString queueName,
callBrokerQueuePlugins(plugin -> plugin.afterDestroyQueue(queue, address, session, checkConsumerCount, removeConsumers, autoDeleteAddress));
}
- if (queue.isTemporary()) {
- AddressInfo addressInfo = getAddressInfo(address);
+ AddressInfo addressInfo = getAddressInfo(address);
- if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
- try {
- removeAddressInfo(address, session);
- } catch (ActiveMQDeleteAddressException e) {
- // Could be thrown if the address has bindings or is not deletable.
- }
+ if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
Review comment:
If that's the case, then I'm not sure what the `autoDeleteAddress` flag actually does anymore. It seems like we should just remove it.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-956325821
actually... that's still not correct.. as destroyQueue would make it remove it immediately... I will handle this any way.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-956324469
@jbertram my issue was with the remove of the temporary queue from the equation... it will probably break a test.
I will pick your change and test with temporary queues and address if any issues.
--
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#discussion_r737488446
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -2397,15 +2397,13 @@ public void destroyQueue(final SimpleString queueName,
callBrokerQueuePlugins(plugin -> plugin.afterDestroyQueue(queue, address, session, checkConsumerCount, removeConsumers, autoDeleteAddress));
}
- if (queue.isTemporary()) {
- AddressInfo addressInfo = getAddressInfo(address);
+ AddressInfo addressInfo = getAddressInfo(address);
- if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
- try {
- removeAddressInfo(address, session);
- } catch (ActiveMQDeleteAddressException e) {
- // Could be thrown if the address has bindings or is not deletable.
- }
+ if (autoDeleteAddress && postOffice != null && addressInfo != null && addressInfo.isAutoCreated() && !isAddressBound(address.toString()) && addressSettingsRepository.getMatch(address.toString()).getAutoDeleteAddressesDelay() == 0) {
Review comment:
I wouldn't do that here.
Removing auto-created queues immediately could break a lot of stuff.
you should instead make sure the AddressReaper would remove such addresses.. I would revert this fix here and would add a check for auto-delete address on the reaper.
--
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
[GitHub] [activemq-artemis] gemmellr commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-952692716
The change looks reasonable in terms of behaviour for the management issue that was raised, but I wonder about its impact on the non-management cases.
It seems this method is also used during operation of the general queue/address auto-deletion sweeps added in ARTEMIS-3502, passing the current address settings auto-deletion config to the queue deletion operation. I'd guess thats likely partly why the change causing this issue was originally made, to stop the address being immediately deleted along with the queue, but rather handled separately (but typically right afterwards) by the address sweeping process with the same '2 sweeps then delete' behaviour to stop churn. Obviously that doesnt work for the management case when that sweep process is disabled, as was seen.
Perhaps the sweeper or management bits need their own method/arg to govern the behaviour to their needs?
--
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
[GitHub] [activemq-artemis] gemmellr commented on pull request #3819: ARTEMIS-3541 createQueue ignoring autoDeleteAddress flag
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3819:
URL: https://github.com/apache/activemq-artemis/pull/3819#issuecomment-952694319
The JIRA + PR title and commit message need tweaked, it is destroyQueue rather than createQueue that is ignoring the flag.
--
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