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