Github user mmiklavc commented on a diff in the pull request: https://github.com/apache/metron/pull/851#discussion_r154230654 --- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java --- @@ -343,25 +343,57 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor * @param type config type to upload configs for * @param configName specific config under the specified config type */ - public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client, - ConfigurationType type, Optional<String> configName) throws Exception { + public static void uploadConfigsToZookeeper( + String rootFilePath, + CuratorFramework client, + ConfigurationType type, + Optional<String> configName) throws Exception { + switch (type) { + case GLOBAL: final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath); if (globalConfig.length > 0) { setupStellarStatically(client, Optional.of(new String(globalConfig))); writeGlobalConfigToZookeeper(globalConfig, client); } break; - case PARSER: // intentional pass-through - case ENRICHMENT: // intentional pass-through - case INDEXING: - Map<String, byte[]> sensorIndexingConfigs = readSensorConfigsFromFile(rootFilePath, type, - configName); - for (String sensorType : sensorIndexingConfigs.keySet()) { - writeConfigToZookeeper(type, configName, sensorIndexingConfigs.get(sensorType), client); + + case PARSER: { + Map<String, byte[]> configs = readSensorConfigsFromFile(rootFilePath, PARSER, configName); --- End diff -- I remember struggling through this class when applying the patching mechanism, so I want to start by saying I'm a big fan of this work and conversation around how to best improve and simplify this class. @cestella I agree with @nickwallen about option 1. Compared to your PR on this PR (sounds like an XZibit meme...) for option 3, I think it's much less intuitive to read. I took a look through it and that would be my preference for the way to go. It's classic strategy pattern through composition, and it eliminates the duplication we see below. I tend to agree with the comments on that PR about eventually pulling those implementations into their own classes. That would make the code much more readable. And I think it probably puts us in a better place for cleaning up the config utils class or removing/renaming it altogether. The only part of this that is still a little bit of a smell to me is that the global config and profiler throw unsupported operation exceptions. I think this i s a big enough improvement that I'd be willing to let that slide for now, but ideally the global config would exist similarly to the other config. e.g. instead of this: - config/zookeeper/ - global.json - profiler.json - parsers/ - ... - enrichment/ - ... - indexing/ - ... - .... etc. You'd give the global config and profiler a directory as well, which would enable a consistent layout on the local FS and in Zookeeper, and a consistent interface for these commands. - config/zookeeper/ - global/ - global.json - profiler/ - profiler.json - parsers/ - ... Then this ``` public interface ConfigurationOperations { ... void writeSensorConfigToZookeeper(String sensorType, byte[] configData, CuratorFramework client) throws Exception; ... ``` could become this ``` public interface ConfigurationOperations { ... void writeConfigToZookeeper(String type, byte[] configData, CuratorFramework client) throws Exception; ``` I'm not sure it's desirable or not, but you could then theoretically provide multiple configs in global/ and profiler/, which is consistent with the parser/, enrichment/, and indexing/ configs.
---