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 #923: DRILL-5723: Added System Internal Options That can ...
Date Sun, 10 Sep 2017 00:33:33 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/923#discussion_r137937100
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java
---
    @@ -17,44 +17,84 @@
      */
     package org.apache.drill.exec.server.options;
     
    -import org.apache.drill.exec.server.options.TypeValidators.BooleanValidator;
    -import org.apache.drill.exec.server.options.TypeValidators.DoubleValidator;
    -import org.apache.drill.exec.server.options.TypeValidators.LongValidator;
    -import org.apache.drill.exec.server.options.TypeValidators.StringValidator;
    -
    -public abstract class BaseOptionManager implements OptionSet {
    -//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class);
    -
    -  /**
    -   * Gets the current option value given a validator.
    -   *
    -   * @param validator the validator
    -   * @return option value
    -   * @throws IllegalArgumentException - if the validator is not found
    -   */
    -  private OptionValue getOptionSafe(OptionValidator validator)  {
    -    OptionValue value = getOption(validator.getOptionName());
    -    return value == null ? validator.getDefault() : value;
    +import org.apache.drill.common.exceptions.UserException;
    +
    +import java.util.Iterator;
    +
    +/**
    + * This {@link OptionManager} implements some the basic methods and should be extended
by concrete implementations.
    + */
    +public abstract class BaseOptionManager extends BaseOptionSet implements OptionManager
{
    +  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class);
    +
    +  @Override
    +  public OptionList getInternalOptionList() {
    +    return getAllOptionList(true);
       }
     
       @Override
    -  public boolean getOption(BooleanValidator validator) {
    -    return getOptionSafe(validator).bool_val;
    +  public OptionList getPublicOptionList() {
    +    return getAllOptionList(false);
       }
     
       @Override
    -  public double getOption(DoubleValidator validator) {
    -    return getOptionSafe(validator).float_val;
    +  public void setLocalOption(String name, boolean value) {
    +    setLocalOption(OptionValue.Kind.BOOLEAN, name, Boolean.toString(value));
    --- End diff --
    
    Very nice improvement to the API! I wonder, however, if we should pass the value as an
`Object` rather than as a `String`? Seems more natural to do it that way. Or, maybe here just
create the option value since we know the type?
    
    ```
    setLocalOption(String name, OptionValue.fromBoolean(value));
    ```
    
    This means that the other fields would be filled in later, so they couldn't be final.
    
    Hence, the alternative, use an `Object`
    
    ```
    setLocalOption(String name, value);
    ...
    
    private void setLocalOption(String name, Object value) {
    ```
    
    Then, when creating an option, validate that the type of the `Object` matches the type
of the option. (In fact, the `Object` for would be handy for testing...)



---

Mime
View raw message