Repository navigation
Conversation
ppkarwasz
left a comment
ppkarwasz
left a comment
There was a problem hiding this comment.
Hi @rgoers,
As currently written, only a single YAML file can be used as source. Is this intentional? If we use Java *.properties file, Flume will ignore and log loading errors for distinct configuration files. Is this by design or an uncaught bug?
| <groupId>org.snakeyaml</groupId> | ||
| <artifactId>snakeyaml-engine</artifactId> | ||
| <version>${snake-yaml.version}</version> | ||
| <optional>true</optional> |
There was a problem hiding this comment.
I think it is fine to add it as required dependency.
| boolean isYaml; | ||
| try { | ||
| Class.forName("org.snakeyaml.engine.v2.api.Load"); | ||
| isYaml = true; | ||
| } catch (Exception exception) { | ||
| isYaml = false; | ||
| } | ||
| isYamlSupported = isYaml; |
There was a problem hiding this comment.
…which of course will remove the need for reflection.
| case ConfigurationSource.YAML: | ||
| case ConfigurationSource.YML: { | ||
| if (isYamlSupported) { | ||
| properties = YamlConfigConverter.convertYamlToProperties(is); |
There was a problem hiding this comment.
We should append the properties from multiple sources instead of replacing them.
| properties = YamlConfigConverter.convertYamlToProperties(is); | |
| properties.putAll(YamlConfigConverter.convertYamlToProperties(is)); |
There was a problem hiding this comment.
This is going to be my next PR.
| Load load = new Load(settings); | ||
|
|
||
| // 2. Load the YAML content into basic Java structures | ||
| Object yamlData = load.loadFromInputStream(yamlInputStream); |
There was a problem hiding this comment.
This can throw a YamlEngineException, which, unlike Properties.load will not be caught in the UriConfigurationProvider loop.
If one source is defective, the entire configuration will fail.
There was a problem hiding this comment.
I will fix this in the next PR.
|
Closing this since all the changes were incorporated in 515. |
Also fixes some minor PMD issues.