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 2020/09/24 16:33:59 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
gtully opened a new pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277
…atching page-store-name address setting
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#issuecomment-698456000
@clebertsuconic is there a better place for this check? It seems as if addressInfo should know about wildcards but that logic is currently in addressImpl and the address manager.
hmm, maybe it also needs to be conditional on wildcard routing being enabled, maybe folks run with it disabled for a tiny perf lift
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494465252
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
I share that concern, i did not find a way to get the AddressImpl back from the binding created by the wildcard address manager. It needs some surgery to access.
The addressImpl has a reference to the wildcard config, I guess I could duplicate that logic in place of the new ..
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
Thank you, that is a nice improvement 😷
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494498889
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
@gtully I've sent a Pull Request towards your personal fork:
https://github.com/gtully/activemq-artemis/pull/1
if you can squash it with your commit. I was looking through the code it was easier to just write the change instead of explaining 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494462075
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
Review comment:
I don't think you need to check for warn enabled... it shouldn't be a big burden anyways.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#issuecomment-698456000
@clebertsuconic is there a better place for this check? It seems as if addressInfo should know about wildcards but that logic is currently in addressImpl and the address manager.
hmm, maybe it also needs to be conditional on wildcard routing being enabled, maybe folks run with it disabled for a tiny perf lift
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494465252
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
I share that concern, i did not find a way to get the AddressImpl back from the binding created by the wildcard address manager. It needs some surgery to access.
The addressImpl has a reference to the wildcard config, I guess I could duplicate that logic in place of the new ..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494502628
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
actually, I will merge with my change squashed...
you can follow up with additional commits if you don't agree with anything.
thanks a lot for this PR.. very nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494462075
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
Review comment:
I don't think you need to check for warn enabled... it shouldn't be a big burden anyways.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
isn't there a better way instead of new AddressImpl() ?
that's the only concern I have.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
@gtully I've sent a Pull Request towards your personal fork:
https://github.com/gtully/activemq-artemis/pull/1
if you can squash it with your commit. I was looking through the code it was easier to just write the change instead of explaining it.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
actually, I will merge with my change squashed...
you can follow up with additional commits if you don't agree with anything.
thanks a lot for this PR.. very nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494462819
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
isn't there a better way instead of new AddressImpl() ?
that's the only concern I have.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3277: ARTEMIS-2768 - add warning if wildcard subscription does not have a m…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3277:
URL: https://github.com/apache/activemq-artemis/pull/3277#discussion_r494530379
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
##########
@@ -3512,7 +3513,14 @@ public Queue createQueue(final QueueConfiguration queueConfiguration, boolean ig
}
}
- QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString()));
+ final AddressSettings addressSettings = addressSettingsRepository.getMatch(getRuntimeTempQueueNamespace(queueConfiguration.isTemporary()) + queueConfiguration.getAddress().toString());
+ QueueConfigurationUtils.applyDynamicQueueDefaults(queueConfiguration, addressSettings);
+
+ if (ActiveMQServerLogger.LOGGER.isEnabled(Logger.Level.WARN)) {
+ if (new AddressImpl(queueConfiguration.getAddress(), configuration.getWildcardConfiguration()).containsWildCard() && addressSettings.getPageStoreName() == null) {
Review comment:
Thank you, that is a nice improvement 😷
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org