drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From paul-rogers <...@git.apache.org>
Subject [GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Date Tue, 11 Jul 2017 23:54:49 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/868#discussion_r126833530
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
    @@ -314,6 +370,31 @@ public void deleteAllOptions(OptionType type) {
         }
       }
     
    +  public void populateDefualtValues(Map<String, OptionValidator> VALIDATORS) {
    --- End diff --
    
    Nit: the `VALIDATORS` variable is not a constant here, so we should call it something
like `validators`.
    
    Or, since `VALIDATORS` is a static member, we can access it directly; no need to pass
it into this method.
    
    Moreover, since `VALIDATORS` is static, this whole method can be static. It should be
called (once) from somewhere in, say, the Drillbit startup logic. But, since we can have multiple
drillbits in the same process (when testing), we should perhaps have a flag so that initialization
is done only on the first call, and synchronized so that two Drillbits don't try to do the
same init at the same time.
    
    Or, we might want to have a separate table per Drillbit. Maybe the items should become
a non-static member of the SessionOptions class, if we have one SessionOptions per Drillbit.
That way we can test strange scenarios, such as two Drillbits having different defaults (which
is a bug, but we might want to test if we can catch that improper setup...)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message