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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
