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.


---

Reply via email to