ARTEMIS-5871 restrict reload of properties config to the set of reloa… #6205
Conversation
|
There is a use case where a user has a coded configuration impl that they pass to the broker, if that contains re loadable config and there are the same entries in a properties file that is reloaded, then the reload will take precedence. In this case there are two sources of truth and one is re-loadable, I think this is the expected behaviour, but due to this bug, it is not currently the case. |
...mis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java
Outdated
Show resolved
Hide resolved
It wont just 'take precedence', it could silently toss original configuration even where they dont overlap at all, giving potentially a completely different result than when the very same properties were applied at startup. So even touching the file could alter config vs startup, which itself seems like an issue. FWIW, you added a startup class for use with images thats specifically using that behaviour, and added ability to change the properties path on another related embedding class that then allows it too, so youll likely need to look at updating those, and some of the tests that use it, if you want to change this. |
|
configurationFileRefreshPeriod=-1 is necessary if there are multiple source of truth for config that supports reload. |
|
@gemmellr I peeked at the image example tests, none have overlapping re-loadable config. I cannot find any broken usage in our test suite. |
...mis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java
Show resolved
Hide resolved
| configuration.parseProperties(propertiesFileUrl); | ||
| config.parseProperties(propertiesFileUrl); | ||
| configuration.setStatus(config.getStatus()); | ||
|
|
There was a problem hiding this comment.
This seems to disregard any configuration that was assigned programmatically via an initial configuration object when it sets the configuration from the reload of the possible XML file and then the properties file. It creates an empty configuration vessel which it might fill in from XML and or a properties file(s) set and then hard sets those entries into the existing configuration regardless of the differences between old and new which could lose original data. Its not clear when or why these differing configuration sets take precedence over each other.
There was a problem hiding this comment.
that is exactly the intent, the new config is just for the purpose of being reloaded by the reload logic. There is currently no callback for programatic config. Typically broker config is read once at startup. The reload logic is restricted to the set of accessors that support reload, the bits that are copied over after a config is reread. Then the deployReloadableConfigFromConfiguration method does the heavy lifting of applying any changes or not to individual components.
So the difference check is all in deployReloadableConfigFromConfiguration
The config doesnt actually need to 'overlap' for it to be a problem, its arguably even more of an issue when it doesnt as then the original certainly shouldnt be getting tossed. The issue is also not going to be contained to our test suite. We provide multiple APIs that allow providing Configuration objects, even encourage it, or always do it. Your changes dont even try to stop that, they'll just make it silently do something egregiously different than before.
Again, this is not an issue contained to our test suite. Check out the main ActiveMQServerImpl constructors, or the 'Main' which do supply a Configuration, or EmbeddedActiveMQ which it uses and similarly can or will do the same, and is used in various other places to that end. |
This has never been a stated requirement and still wouldnt be enforced (which it should be if its a requirement), and I know you originally added the broker properties to use in a situation there were specifically multiple sources (with updates to each targeting different configuration areas, i.e not overlapping) so that seems strange to say now. I can see it causes problems for your current changes, but I dont think silently breaking stuff that has been working is necessarily the way to go. I also think what these changes would seem to allow in later silently tossing startup configuration would be a worse bug than any consideration of the way it currently behaves as being a bug. |
|
This is not a change of expected behaviour, config reload has always tossed startup config. That is how it works, such that the config taken at startup can be compared to the new config. |
...mis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java
Outdated
Show resolved
Hide resolved
We have a different definition of 'change' then. It has been the case that programmatic config which hasnt changed by the properties has been safe from properties config reload previously, given the way the properties config [reload] always worked at startup and later. It plays partly into the same space that meant you previously added a mechanism that allow 'unsetting' existing config by adding a 'removal property' value, which is partly what you look to be trying to change here. |
|
I don't get the "silently breaking what has been working", it is fixing what has not been working. |
…dable config elements
|
It doesnt make sense for config reload with the same unchanged config properties to result in a different effective config than startup does from the very same config/properties. Yet for a provided Configuration thats something your changes can do, throwing away the initial configuration in the reloadable sections, totally unlike it does at startup, and unlike what it did before. Thats a far worse behaviour than anything. The embedded "Main" class that was initially aimed at usage with properties to add additional config, to some existing config. That for example supplies the initial broker config as a Configuration object (loaded from an XML file, but not by the broker itself). It uses the EmbeddedActiveMQ to do that, which you also added support for providing a specified properties to go with existing base provided Configuration. The broker itself has for years automatically loaded a properties file (if found) at startup and applies it as additional configuration to whatever the existing passed in (or parsed) Configuration was. Your change will take those same configuration details at reload, potentially completely unchanged, and then do something entirely different with it by tossing the original Configuration in the reloadable sections and using only the effect of the properties alone, resulting in a completely different effective config from the same starting configuration details even though noe of them actually changed. Something that is currently functioning in a reasonably expected manner, in the way it always worked, and changes to do something thats quite unexpected, and doesnt even say its doing it. Thats what I mean by silently breaking what has been working. |
|
I agree that config reload with a provided programmatic config is broken. There are no callbacks and have been no callbacks for programmatic config to get reapplied. That is not an argument for not restricting the reload of properties config to re loadable components. The fact that the effective config was changing on reload is irrelevant unless something reads the new config. The only thing that rereads config are the components that support config reload when they are poked on reload. |
3 participants
…dable config elements