You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "brusdev (via GitHub)" <gi...@apache.org> on 2024/01/04 14:32:03 UTC
[PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
brusdev opened a new pull request, #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729
(no comment)
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1453163076
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/CacheConfiguration.java:
##########
@@ -43,4 +52,12 @@ public CacheConfiguration setTimeout(int timeout) {
this.timeout = timeout;
return this;
}
+
+ public CacheConfiguration() {
+
+ }
+
+ public CacheConfiguration(boolean enabled) {
+ this.enabled = enabled;
+ }
Review Comment:
Can we put the constructors at the top given their importance in this case (will also minimise the diff in this case as one was there already)
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java:
##########
@@ -2871,6 +2871,9 @@ private void parseConnectionRouterConfiguration(final Element e, final Configura
}
private void parseCacheConfiguration(final Element e, final CacheConfiguration cacheConfiguration) throws ClassNotFoundException {
+ cacheConfiguration.setEnabled(getBoolean(e, "enabled",
+ cacheConfiguration.isEnabled()));
Review Comment:
Feels like this would be better defaulting to simply the literal "true" per the xsd as you had it originally?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/PoolConfiguration.java:
##########
@@ -118,4 +129,12 @@ public PoolConfiguration setDiscoveryGroupName(String discoveryGroupName) {
this.discoveryGroupName = discoveryGroupName;
return this;
}
+
+ public PoolConfiguration() {
+
+ }
+
+ public PoolConfiguration(boolean enabled) {
+ this.enabled = enabled;
+ }
Review Comment:
Ditto
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java:
##########
@@ -2889,6 +2892,9 @@ private void parsePolicyConfiguration(final Element e, final NamedPropertyConfig
}
private void parsePoolConfiguration(final Element e, final Configuration config, final PoolConfiguration poolConfiguration) throws Exception {
+ poolConfiguration.setEnabled(getBoolean(e, "enabled",
+ poolConfiguration.isEnabled()));
Review Comment:
ditto
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1449339897
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I found this bit particularly confusing as it defaults the value in the constituent configuration objects to true but then forces them to false here. My assumption is that this is somehow tied to trying to make this work both in the XML configuration and the broker properties bits but I'm not 100% its consistent between the two. There doesn't appear to be an enable option in the XML but instead seems to imply that the presence alone is enabling but the properties would require the option to be set true even if any of the other configuration is present.
In general I'd prefer if the code used a simpler default to false approach and the configuration options where consistent or could be changed such that the user gets a default enabled from XML via the file parser using a default and / or making it configurable in XML. Then it would be nice also doing something consistent in the broker properties. Just looking at the code changes to this point set of red flags for me and I had to chase down why it did this.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#issuecomment-1898798243
that looks good to me. it makes sense that the properties applier will auto create default instances for any intermediary that it encounters while trying to apply nested properties.
What we may need is a test for the case where w.x.y.z=1 is set and both getX and getY can return null.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1450218942
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I had the same reaction as Tim to the original code but hadnt got to thinking of an improvement.
In some ways this is definitely better, the code is a little more consistent with itself and other previous code. In others its much the same as before; the 'enabled' bit can now be controlled in both configs rather than just one, but there is still effectively 3 different/conflicting 'default' behaviours for it between XML and broker-properties and both, which is confusing. But I dont necessarily have an idea thats nicer to resolve that while there are actually 3 different ways of configuring things.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1450611631
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I'm happy enough that it is at least more obvious now that it is more visible that it is consistently inconsistent between the various configuration paths. Other than more documentation surrounding those inconsistencies I'm not sure how we could fix that sadly.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1453038703
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I reverted the default value of the `enabled` bit for cache and pool configurations to true but I keep the xml attribute `enabled` suggested by @tabish121. The 3 different ways of configuring things seem more consistent:
- xml: the `connection-router` element has no cache or pool enabled by default but if you create a cache or pool element they are enabled by default (this is important for backward compatibility)
- programmatically: the `ConnectionRouterConfiguration` class has no cache or pool enabled by default but if you create a CacheConfiguration or PoolConfiguration class they are enabled by default (this is important for backward compatibility)
- broker properties: the `connectionRouters` items have no cache or pool enabled by default but you can set enabled to true to enable them
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#issuecomment-1898699450
LGTM from my minimal understanding of the properties magic.
@gtully thoughts?
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1449339897
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I found this bit particularly confusing as it defaults the value in the constituent configuration objects to true but then forces them to false here. My assumption is that this is somehow tied to trying to make this work both in the XML configuration and the broker properties bits but I'm not 100% sure its consistent between the two. There doesn't appear to be an enable option in the XML but instead seems to imply that the presence alone is enabling but the broker properties path would require the option to be set true even if any of the other configuration is present.
In general I'd prefer if the code used a simpler default to false approach and the configuration options were consistent or could be changed such that the user gets a default enabled from XML via the file parser using a default and / or making it configurable in XML. Then it would be nice also doing something consistent in the broker properties. Just looking at the code changes to this point set of red flags for me and I had to chase down why it did this.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#issuecomment-1898733016
> LGTM from my minimal understanding of the properties magic.
>
> @gtully thoughts?
I like the idea, although I remain mostly befuddled by the broker properties code.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1453038703
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I reverted the default value of the `enabled` bit for cache and pool configurations to true but I kept the xml attribute `enabled` suggested by @tabish121. The 3 different ways of configuring things seem more consistent:
- xml: the `connection-router` element has no cache or pool enabled by default but if you create a cache or pool element they are enabled by default (this is important for backward compatibility)
- programmatically: the `ConnectionRouterConfiguration` class has no cache or pool enabled by default but if you create a CacheConfiguration or PoolConfiguration class they are enabled by default (this is important for backward compatibility)
- broker properties: the `connectionRouters` items have no cache or pool enabled by default but you can set enabled to true to enable them
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr merged PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1449968857
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
@tabish121 I like your solution, it is more consistent. I changed the implementation.
--
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
Re: [PR] ARTEMIS-4552 Config all connection-router settings by using broker properties [activemq-artemis]
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4729:
URL: https://github.com/apache/activemq-artemis/pull/4729#discussion_r1457483882
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/routing/ConnectionRouterConfiguration.java:
##########
@@ -26,8 +26,8 @@ public class ConnectionRouterConfiguration implements Serializable {
private KeyType keyType = KeyType.SOURCE_IP;
private String keyFilter = null;
private String localTargetFilter = null;
- private CacheConfiguration cacheConfiguration = null;
- private PoolConfiguration poolConfiguration = null;
+ private CacheConfiguration cacheConfiguration = new CacheConfiguration().setEnabled(false);
+ private PoolConfiguration poolConfiguration = new PoolConfiguration().setEnabled(false);
Review Comment:
I completely removed the `enabled` attribute.
--
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