[ 
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16083207#comment-16083207
 ] 

ASF GitHub Bot commented on DRILL-5547:
---------------------------------------

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


> Drill config options and session options do not work as intended
> ----------------------------------------------------------------
>
>                 Key: DRILL-5547
>                 URL: https://issues.apache.org/jira/browse/DRILL-5547
>             Project: Apache Drill
>          Issue Type: Bug
>          Components:  Server
>    Affects Versions: 1.10.0
>            Reporter: Karthikeyan Manivannan
>            Assignee: Venkata Jyothsna Donapati
>             Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But 
> several of these session options are assigned hard-coded default values when 
> the option validators are initialized. Because of this config options will 
> never be read and honored even if the user did not specify the session 
> option. 
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as 
> the default value. This default value gets into the session options map via 
> the initialization of validators in SystemOptionManager. 
> Now any piece of code that tries to check if a session option is set will 
> never see a null, so it will always use that value and never try to look into 
> the config options. For example, in the following piece of code from 
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ? 
> value.string_val.toUpperCase() : 
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Reply via email to