Github user HeartSaVioR commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2445#discussion_r155708774
  
    --- Diff: 
flux/flux-core/src/main/java/org/apache/storm/flux/parser/FluxParser.java ---
    @@ -39,51 +39,57 @@
     
         private FluxParser(){}
     
    -    // TODO refactor input stream processing (see parseResource() method).
         public static TopologyDef parseFile(String inputFile, boolean 
dumpYaml, boolean processIncludes,
                String propertiesFile, boolean envSub) throws IOException {
    -   
    +
             FileInputStream in = new FileInputStream(inputFile);
    -        TopologyDef topology = parseInputStream(in, dumpYaml, 
processIncludes, propertiesFile, envSub);
    +        InputStream propertiesIn = null;
    +        if (propertiesFile != null) {
    +            propertiesIn = new FileInputStream(propertiesFile);
    +        }
    +        TopologyDef topology = parseInputStream(in, dumpYaml, 
processIncludes, propertiesIn, propertiesFile, envSub);
             in.close();
    -        
    +
             return topology;
         }
     
         public static TopologyDef parseResource(String resource, boolean 
dumpYaml, boolean processIncludes,
                String propertiesFile, boolean envSub) throws IOException {
             
             InputStream in = FluxParser.class.getResourceAsStream(resource);
    -        TopologyDef topology = parseInputStream(in, dumpYaml, 
processIncludes, propertiesFile, envSub);
    +        InputStream propertiesIn = null;
    +        if (propertiesFile != null) {
    +            propertiesIn = 
FluxParser.class.getResourceAsStream(propertiesFile);
    +        }
    +        TopologyDef topology = parseInputStream(in, dumpYaml, 
processIncludes, propertiesIn, propertiesFile, envSub);
    --- End diff --
    
    There's a corner case here:
    
    1. `--resource` option is provided hence this method called.
    2. yaml file has an include statement which sets resource as `false`.
    3. `parseFile` is called which doesn't handle propertiesFile as resource.
    
    So it needs more fine-grained approach regarding dealing with 
propertiesFile.
    
    If my understanding is right, given that we only read and don't modify 
properties file after loading it, we could just create Properties instance and 
pass it instead of passing properties file name. Makes sense?


---

Reply via email to