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.
---
