gemmellr commented on code in PR #6205:
URL: https://github.com/apache/artemis/pull/6205#discussion_r2804697585
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/ConfigurationTest.java:
##########
@@ -350,6 +374,55 @@ public void
testPropertiesDirWithFilterConfigReloadOnNewFileAfterGettingJournalL
}
}
+ @Test
+ public void testPropertiesAndProgrammaticReloadableConfigArg() throws
Exception {
+
+ File propsFile = new File(getTestDirfile(), "somemore.props");
+ propsFile.createNewFile();
+
+
+ Properties properties = new
ConfigurationImpl.InsertionOrderedProperties();
+ properties.put("configurationFileRefreshPeriod", "100");
+ properties.put("persistenceEnabled", "false");
+ properties.put("acceptorConfigurations.reloadable.factoryClassName",
NETTY_ACCEPTOR_FACTORY);
+ properties.put("acceptorConfigurations.reloadable.params.HOST",
"LOCALHOST");
+ properties.put("acceptorConfigurations.reloadable.params.PORT", "61616");
+
+ try (FileOutputStream outStream = new FileOutputStream(propsFile)) {
+ properties.store(outStream, null);
+ }
+ assertTrue(propsFile.exists());
+
+ ConfigurationImpl programmatic = new ConfigurationImpl();
+ programmatic.addAcceptorConfiguration("tcp", "tcp://localhost:62618");
+ ActiveMQJAASSecurityManager sm = new
ActiveMQJAASSecurityManager(InVMLoginModule.class.getName(), new
SecurityConfiguration());
+ ActiveMQServer server = addServer(new ActiveMQServerImpl(programmatic,
sm));
+
+ assertThrows(IllegalArgumentException.class, () -> {
+ server.setProperties(propsFile.getAbsolutePath());
+ });
Review Comment:
This doesnt really make sense. You explicily check it throws IAE (again ISE
seems more in line), but then the test proceeds and acts as if this was a
perfectly valid argument and the IAE didnt happen, and explicitly checks the
'new properties get reloaded and tosses away all original configuration' thing
that didnt used to happen.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/ConfigurationTest.java:
##########
@@ -199,12 +208,13 @@ public void testPropertiesOnlyConfigReload() throws
Exception {
assertEquals(1,
server.getConfiguration().getConnectionRouters().size());
assertEquals("LF",
server.getConfiguration().getConnectionRouters().get(0).getLocalTargetFilter());
-
- properties.put("persistenceEnabled", "false");
- properties.put("configurationFileRefreshPeriod", "100");
-
+ assertEquals(1,
server.getActiveMQServerControl().getAcceptors().length);
// verify update
+ properties.put("configurationFileRefreshPeriod", "100");
Review Comment:
Seems like either some comment needed to explain better, or the properties
clear should be here, or better a fresh set of properties used.
Could probably do with a comment either way to describe the changes such
this is both 'update' and the later 'remove' changes.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -681,6 +682,7 @@ public CriticalAnalyzer getCriticalAnalyzer() {
@Override
public void setProperties(String fileUrltoBrokerProperties) {
propertiesFileUrl = fileUrltoBrokerProperties;
+
throwIfReloadableConfigProvidedWithoutFileAndBrokerPropertiesUrlNonNullAndReload(configuration,
propertiesFileUrl);
Review Comment:
The check should be _before_ the path is set, otherwise it is ineffective
and acts as if this is a perfectly valid argument (as the later test shows).
The exception should be documented.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/routing/ElasticQueueTest.java:
##########
@@ -427,6 +427,7 @@ private void prepareNodesAndStartCombinedHeadTail() throws
Exception {
.setAutoDeleteQueues(false).setAutoDeleteAddresses(false); // so slow
consumer can kick in!
Configuration baseConfig = new ConfigurationImpl();
+ baseConfig.setConfigurationFileRefreshPeriod(-1); // the classpath has
broker.properties we don't want to reload
Review Comment:
Then it probably shouldnt since that would mean we dont actually know the
current config of the broker during most of the tests.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/ConfigurationUtils.java:
##########
@@ -149,6 +149,25 @@ public static void validateConfiguration(Configuration
configuration) {
compareTTLWithCheckPeriod(configuration);
}
+ public static void
throwIfReloadableConfigProvidedWithoutFileAndBrokerPropertiesUrlNonNullAndReload(Configuration
configuration, String propertiesFileUrl) {
+ if (configuration.getConfigurationUrl() == null &&
configuration.getConfigurationFileRefreshPeriod() > 0 &&
configuration.resolvePropertiesSources(propertiesFileUrl) != null) {
+ // if any non xml (programmatic) provided config is reloadable, on
the first properties source reload it will whack that config as the reload has
the source of truth for reloadable attributes
+ if (!configuration.getSecurityRoles().isEmpty() ||
+ !configuration.getAddressSettings().isEmpty() ||
+ !configuration.getDivertConfigurations().isEmpty() ||
+ !configuration.getAddressConfigurations().isEmpty() ||
+ !configuration.getQueueConfigs().isEmpty() ||
+ !configuration.getBridgeConfigurations().isEmpty() ||
+ !configuration.getConnectorConfigurations().isEmpty() ||
+ !configuration.getAcceptorConfigurations().isEmpty() ||
+ !configuration.getAMQPConnection().isEmpty() ||
+ !configuration.getConnectionRouters().isEmpty()) {
+
+ throw new IllegalArgumentException(String.format("a properties
source (%s) is illegal, programmatic config contains reloadable elements and
configurationFileRefreshPeriod > 0; your programmatic config will be replaced
on reload of the properties source", propertiesFileUrl));
Review Comment:
Feels like IllegalState might be more appropriate since this is used in the
setProperties(..) method and the very same properties path argument could be
valid depending on the existing config/state.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/ConfigurationUtils.java:
##########
@@ -149,6 +149,25 @@ public static void validateConfiguration(Configuration
configuration) {
compareTTLWithCheckPeriod(configuration);
}
+ public static void
throwIfReloadableConfigProvidedWithoutFileAndBrokerPropertiesUrlNonNullAndReload(Configuration
configuration, String propertiesFileUrl) {
+ if (configuration.getConfigurationUrl() == null &&
configuration.getConfigurationFileRefreshPeriod() > 0 &&
configuration.resolvePropertiesSources(propertiesFileUrl) != null) {
+ // if any non xml (programmatic) provided config is reloadable, on
the first properties source reload it will whack that config as the reload has
the source of truth for reloadable attributes
+ if (!configuration.getSecurityRoles().isEmpty() ||
+ !configuration.getAddressSettings().isEmpty() ||
+ !configuration.getDivertConfigurations().isEmpty() ||
+ !configuration.getAddressConfigurations().isEmpty() ||
+ !configuration.getQueueConfigs().isEmpty() ||
+ !configuration.getBridgeConfigurations().isEmpty() ||
+ !configuration.getConnectorConfigurations().isEmpty() ||
+ !configuration.getAcceptorConfigurations().isEmpty() ||
+ !configuration.getAMQPConnection().isEmpty() ||
+ !configuration.getConnectionRouters().isEmpty()) {
Review Comment:
If not the whole method, then at least this bit of it seems like it should
maybe be in the Configuration so its less likely to go stale from not being
updated when the Configuration is.
E.g this is actually incorrect here already, because it does not match what
you do later in the config reload bits below (which points to some missing
testing).
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -4682,22 +4684,28 @@ public void reloadConfigurationFile() throws Exception {
}
private void reloadConfigurationFile(URL xmlConfigUri) throws Exception {
+ Configuration config = new ConfigurationImpl();
if (xmlConfigUri != null) {
- Configuration config = new
FileConfigurationParser().parseMainConfig(xmlConfigUri.openStream());
+ config = new
FileConfigurationParser().parseMainConfig(xmlConfigUri.openStream());
LegacyJMSConfiguration legacyJMSConfiguration = new
LegacyJMSConfiguration(config);
legacyJMSConfiguration.parseConfiguration(xmlConfigUri.openStream());
- configuration.setSecurityRoles(config.getSecurityRoles());
- configuration.setAddressSettings(config.getAddressSettings());
-
configuration.setDivertConfigurations(config.getDivertConfigurations());
-
configuration.setAddressConfigurations(config.getAddressConfigurations());
- configuration.setQueueConfigs(config.getQueueConfigs());
-
configuration.setBridgeConfigurations(config.getBridgeConfigurations());
-
configuration.setConnectorConfigurations(config.getConnectorConfigurations());
-
configuration.setAcceptorConfigurations(config.getAcceptorConfigurations());
-
configuration.setAMQPConnectionConfigurations(config.getAMQPConnection());
- configuration.setPurgePageFolders(config.isPurgePageFolders());
}
- configuration.parseProperties(propertiesFileUrl);
+ config.parseProperties(propertiesFileUrl);
+ configuration.setStatus(config.getStatus());
+
+ configuration.setSecurityRoles(config.getSecurityRoles());
+ configuration.setAddressSettings(config.getAddressSettings());
+ configuration.setDivertConfigurations(config.getDivertConfigurations());
+
configuration.setAddressConfigurations(config.getAddressConfigurations());
+ configuration.setQueueConfigs(config.getQueueConfigs());
+ configuration.setBridgeConfigurations(config.getBridgeConfigurations());
+
configuration.setConnectorConfigurations(config.getConnectorConfigurations());
+
configuration.setAcceptorConfigurations(config.getAcceptorConfigurations());
+
configuration.setAMQPConnectionConfigurations(config.getAMQPConnection());
+ configuration.setPurgePageFolders(config.isPurgePageFolders());
+ configuration.setConnectionRouters(config.getConnectionRouters()); //
needs reload logic
+ configuration.setJaasConfigs(config.getJaasConfigs());
Review Comment:
This doenst properly align with the checks done previously (see earlier
comment).
It also adds [XML] reload (though without logic?) for bits that were not
covered before, which doesnt seem suited to the Jira description and feels
somewhat separate.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
