[
https://issues.apache.org/jira/browse/ARTEMIS-5871?focusedWorklogId=1005108&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1005108
]
ASF GitHub Bot logged work on ARTEMIS-5871:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 13/Feb/26 15:25
Start Date: 13/Feb/26 15:25
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1005108)
Time Spent: 3h 20m (was: 3h 10m)
> reload of broker properties config should be restricted to confined to
> re-loadable components
> ---------------------------------------------------------------------------------------------
>
> Key: ARTEMIS-5871
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5871
> Project: Artemis
> Issue Type: Bug
> Components: Configuration
> Affects Versions: 2.50.0
> Reporter: Gary Tully
> Assignee: Gary Tully
> Priority: Major
> Labels: pull-request-available
> Time Spent: 3h 20m
> Remaining Estimate: 0h
>
> currently on reload config, broker properties are applied to the current
> broker config in error.
> This means that the absence of a value is not reflected in the config update,
> simply removing the properties does not result in a removed component or
> configuration entry.
> This is not consistent with the xml reload but also means for properties only
> config, there needs to be an explicit remove key=- value. which then needs to
> be removed.
> If config reload of properties are confined to a new config that is then
> compared in the normal way with the component reload logic, the properties
> can be the source of truth.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
