Details
-
Bug
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
-
None
Description
After IGNITE-18581 we have started to store local configuration in local config file, instead of vault.
The current flow with the saving configuration to a file has a bug. In the method LocalFileConfigurationStorage#write we call LocalFileConfigurationStorage#saveValues to save configuration fields to a file, where we call LocalFileConfigurationStorage#renderHoconString. Named list value has internal id which is UUID, but com.typesafe do not support UUID, so the whole process of saving configuration to a file fails with
Caused by: com.typesafe.config.ConfigException$BugOrBroken: bug in method caller: not valid to create ConfigValue from: 489e16e8-3123-44a3-b27d-6e410863eb24 at app//com.typesafe.config.impl.ConfigImpl.fromAnyRef(ConfigImpl.java:282) at app//com.typesafe.config.impl.PropertiesParser.fromPathMap(PropertiesParser.java:165) at app//com.typesafe.config.impl.PropertiesParser.fromPathMap(PropertiesParser.java:95) at app//com.typesafe.config.impl.ConfigImpl.fromAnyRef(ConfigImpl.java:265) at app//com.typesafe.config.impl.ConfigImpl.fromPathMap(ConfigImpl.java:201) at app//com.typesafe.config.ConfigFactory.parseMap(ConfigFactory.java:1225) at app//com.typesafe.config.ConfigFactory.parseMap(ConfigFactory.java:1236) at app//org.apache.ignite.internal.configuration.storage.LocalFileConfigurationStorage.renderHoconString(LocalFileConfigurationStorage.java:208) at app//org.apache.ignite.internal.configuration.storage.LocalFileConfigurationStorage.saveValues(LocalFileConfigurationStorage.java:185) at app//org.apache.ignite.internal.configuration.storage.LocalFileConfigurationStorage.write(LocalFileConfigurationStorage.java:138) at app//org.apache.ignite.internal.configuration.ConfigurationChanger.changeInternally0(ConfigurationChanger.java:606) at app//org.apache.ignite.internal.configuration.ConfigurationChanger.lambda$changeInternally$1(ConfigurationChanger.java:541)
More details
The problem is trickier than it may seem.
Configuration storages receive data in "flat" data format, meaning that the entire tree is converted into a list of pairs:
[{ "dot-separated key string", "serializable value" }]
LocalFileConfigurationStorage interprets keys as literal paths in HOCON representation, which is simply not correct. These keys and values also have meta-information, associated with them, such as:
- order of elements in named list configuration
- internal ids for named list elements
To see, what's exactly in there, you may refer to the org.apache.ignite.internal.configuration.tree.NamedListNodeTest. It has everything laid out explicitly.
Proposed fix
Well, the ideal approach would be rendering the configuration more or less the same way, as we do it for REST.
It means calling ConfigurationUtil#fillFromPrefixMap for every local root.
Local roots can be retrieved using ConfigurationModule, by reading them all from the class path.
Resulting nodes are converted to maps using ConverterToMapVisitor. Then maps are converted to HOCON using its own API.
There are several hidden problems here.
we must check, that HOCON preserves order of keys, and that we use linked hash maps in fillFromPrefixMap
EDIT: HOCON sorts keys alphabetically. Ok- ConverterToMapVisitor does not expect null nodes, because it always works with "full" trees. Fixing it would require some fine-tuning, otherwise one may end up with a bunch of empty nodes in the config file, which is bad
- ConverterToMapVisitor uses array syntax for named lists. You can see it in action in HoconConverterTest.
Yes, there are two ways of representing named lists in the system. We should make rendering mode configurable, because local configuration, at the moment, only needs basic tree representation (for node attributes)
We should also add tests for most of these improvements. First of all, to HoconConverterTest.
Misc
Another extremely uncertain thing is the way we handle default values. This may be a topic for another issue, or maybe not.
For example, if user explicitly configure network port to 47500, we will fail to save it into the file, because it matches default and we ignore everything that has default value. We need tests for this.
