DRILL-5547:Linking config options with system option manager #868

dvjyothsna · 2017-07-05T20:26:24Z

Drill has two different config systems each with its own namespace.First being the HOCON based boot time config system.This is a hierarchical system where the top layers override the bottom ones in the following order

Java System Options
distrib.conf
drill-override.conf
drill-module.conf

These are the options that are set before the drill starts.But once drill starts System or session options can be modified using ALTER SYSTEM/SESSION.Even this system provides inheritance sytle in the following order

Session options
System options
Hardcoded defaults

But system/session options have a validator and the validator has a hard coded default value for every option. In the current system validators are registered so that system/session options will always have a default value. So when a system/session options are not explicitly set or a user system/session option is null the hardcoded default was applied since it checks if the option value is null and returns the default set in the validator.But the config options set during boot time are never read and honored since there is no linkage between the two config systems.It is also evident that there are some places where there is some ad-hoc linkage between the two systems.For example, for the code gen compiler, config options are supposed to be read if the system option is not null.But as the validator provides the default values config options are never taken into consideration. The goal of the new system is to link both the systems in such a way that boot-time config options take precedence over the hard coded defaults set in the validator.All the options in the option validator i.e.c options from Exec constants, planner settings etc., are extracted and put into a new name space called drill.exec.options in the .conf file. The default values of the validators in the option validator are populated with the values in the boot-config.This way the values set in the boot time config system are honored.Any user who wish to change the option values in the
config should change the options under the name space drill.exec.options

paul-rogers

Great enhancement!

Provided a number of functional and code style suggestions. Please tackle these and I'll give the feature a second review.

paul-rogers · 2017-07-11T22:29:13Z

exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java

+    // If the max_width is not set in the validator compute from the cpu load average
+    long maxWidth = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE);
+    if(maxWidth == 0) {
+      Double cpu_load_average = optionManager.getOption(ExecConstants.CPU_LOAD_AVERAGE);


Double --> double

paul-rogers · 2017-07-11T22:29:46Z

exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java

+    long maxWidth = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE);
+    if(maxWidth == 0) {
+      Double cpu_load_average = optionManager.getOption(ExecConstants.CPU_LOAD_AVERAGE);
+      this.maxWidthPerNode = (int)  Math.ceil(Runtime.getRuntime().availableProcessors() * cpu_load_average);


this.maxWidthPerNode --> maxWidthPerNode
Here and below.

paul-rogers · 2017-07-11T22:33:25Z

exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java

+    long maxWidth = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE);
+    if(maxWidth == 0) {
+      Double cpu_load_average = optionManager.getOption(ExecConstants.CPU_LOAD_AVERAGE);
+      this.maxWidthPerNode = (int)  Math.ceil(Runtime.getRuntime().availableProcessors() * cpu_load_average);


Probably should:

round result, not take the ceiling

clamp the number at 1 and the number of processors

Math.max(1, Math.min( availProc, Math.round(... * ...) ) )

We didn't do this before, but we should have...

This is true if the value was set by a property, so the clamping should be done regardless of whether the value is retrieved or computed. (Gracefully handle the case where someone sets the value to 0 or 1 million, say.)

paul-rogers · 2017-07-11T22:35:02Z

exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java

   * Limits the maximum level of parallelization to this factor time the number of Drillbits
   */
+  String CPU_LOAD_AVERAGE_KEY = "planner.cpu_load_average";
+  DoubleValidator CPU_LOAD_AVERAGE = new DoubleValidator(CPU_LOAD_AVERAGE_KEY,0.70);


Just a bit confused here. The defaults are moving into the drill-module.conf file, which is good.

But, we leave hard-coded values in ExecConstants?

The result will be that future developers will continue to set the defaults in code, as ever, and not realize that they should set the values in the conf file. What happens if the conf file has one value, the hard-coded defaults another? Quite the riddle...

paul-rogers · 2017-07-11T22:46:24Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java

+   *
+   * @return result default option value
+   */
+  public abstract OptionValue loadConfigDefault(DrillConfig bootConfig, String name, String configPath);


Passing in the config is file. Bug, shouldn't the name and config path already be known? The name is the config property name (in most cases) isn't it? The path is fixed and can be constant?

I'd have thought that we'd declare validators something like this:

public static FOO_VALIDATOR = new StringValidator("exec.foo"); public static BAR_VALIDATOR = new IntValidator("exec.bar", "drill.non.standard.name.bar");

Since the validator is given the name, it need not ask for it again here, it would seem...

paul-rogers · 2017-07-11T23:45:43Z

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ExtendedOptionIterator.java

+    public final String name;
+    public final Kind kind;
+    public final OptionType type;
+    public final String string_value;


string_value --> value

Since this is a column name, might as well make it simple.

Also, can we include the data type? Each validator can return its type. For example, LongValidator can return either Long.class (kind of messy), or OptionDataType.LONG (which is a bit less messy). Then, convert the internal type to a nice display name, perhaps mapping to the DrillType. So, a long becomes Bigint or BIGINT.

paul-rogers · 2017-07-11T23:50:48Z

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ExtendedOptionIterator.java

+    List<OptionValue> optionValueList = Lists.newArrayList();
+    OptionValue value;
+    OptionType type;
+    OptionValue[] temp = new OptionValue[3];


I think this may be a bit easier to implement with a map. Add each "inner" option to a map keyed by value. If you find an existing entry, choose the one with higher precedence and replace the map value if needed.

Then, iterate over the map values to get the final objects. Iteration over the values can be done in each call to the next() method of your iterator to avoid the need to build up a list of extended value objects.

paul-rogers · 2017-07-11T23:52:01Z

exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java

      final OptionManager optionManager = queryContext.getOptions();
-      final long maxWidthPerNode = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val;
+      final long maxWidthPerNode;
+      long maxWidth = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE);


This code appears twice. Can we move it into a function so it appears once? Maybe on to a purpose-build Validator class?

paul-rogers · 2017-07-11T23:53:02Z

exec/java-exec/src/main/resources/drill-module.conf

  batch_queue_throttling_threshold: 100
 }
+
+drill.exec.options:  {


Comment to explain what this is? And to remind people how to use it: create a Validator. Create an entry below with the same name as the session/system option, with a default of a compatible type.

paul-rogers · 2017-07-11T23:53:56Z

exec/java-exec/src/main/resources/drill-module.conf

  batch_queue_throttling_threshold: 100
 }
+
+drill.exec.options:  {


Also, any chance we can alphabetize the list? Just run it through the sort command? Will make future human access just a bit easier...

khurram-faraaz · 2017-07-19T05:43:47Z

@dvjyothsna Can you please add unit tests that cover your changes ?

paul-rogers

Coming along very well. Comments here are mostly about fine-tuning the code.

paul-rogers · 2017-07-27T18:51:51Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java

+   */
+  public SystemOptionManager getSystemOptionManager() {
+    final SystemOptionManager systemOptionManager;
+    if(fallback instanceof SessionOptionManager) {


A system option manager should never have a session option manager as a fallback, right? Shouldn't it be the other way around?

Yes, Correct a System Option Manager is not a fallback for session option manager. FragmentOptionManager and SessionOptionManager use SystemOptionManager as the fall back manager so for both FragmentOptionManager and SessionOptionManager fallback is the System Option Manager so it is returned. But in the case of QueryOptionManager, it uses Session Option Manager as the fallback manager
and since Session Option Manager uses System Option Manager as the fallback, we need to go one level deep and fetch System Option Manager from the Session Option Manager.

paul-rogers · 2017-07-27T18:53:25Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionSet.java

+   * @param validator the maxwidth validator
+   * @return the maxwidth value
+   */
+  long getOption(TypeValidators.MaxWidthValidator validator);


Needed? Can't this work if the MaxWidthValidator is an instance of a long validator?

paul-rogers · 2017-07-27T18:54:22Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java

  private final String optionName;
  private final boolean isAdminOption;
-
+  public static final String ConfigPath = "drill.exec.options.";


Java convention is all caps for constants: CONFIG_PATH. Since that name is somewhat generic, maybe OPTION_DEFAULTS_ROOT or some such.

paul-rogers · 2017-07-27T18:55:04Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java

  public abstract Kind getKind();

+  /**
+   * Gets the default result option value for this validator.


"default result option value" --> "default option value"
And below.

paul-rogers · 2017-07-27T18:55:50Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java

+  /**
+   * Sets the default result option value for this validator.
+   */
+  public abstract void setDefaultValue(OptionValue defaultValue);


When would an external bit of code set this option? If this is called only from loadDefault(), go ahead and make the implementation private without a public API method.

paul-rogers · 2017-07-28T21:15:22Z

exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java

+      final long maxWidthPerNode;
+      double cpu_load_average = optionManager.getOption(ExecConstants.CPU_LOAD_AVERAGE);
+      final long maxWidth = optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE);
+      maxWidthPerNode = ExecConstants.MAX_WIDTH_PER_NODE.computeMaxWidth(cpu_load_average,maxWidth);


Move declaration here: final long maxWidthPerNode = ...

paul-rogers · 2017-07-28T21:17:51Z

exec/java-exec/src/main/resources/drill-module.conf

  batch_queue_throttling_threshold: 100
 }
+
+


Perhaps add a bit more info here:
"Defaults for all system and session options. Provided here for easy customization in Drill distributions in the drill-distrib.conf file. Note: users should not set these values in their drill-override.conf file; users should use ALTER SYSTEM and ALTER SESSION to set the options."

paul-rogers · 2017-07-28T21:19:51Z

exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java

  public void testJDKClassCompiler() throws Exception {
    logger.debug("Testing JDKClassCompiler");
-    sessionOptions.setOption(OptionValue.createString(OptionType.SESSION, ClassCompilerSelector.JAVA_COMPILER_OPTION, ClassCompilerSelector.CompilerPolicy.JDK.name()));
+    sessionOptions.setOption(OptionValue.createString(OptionType.SESSION, ClassCompilerSelector.JAVA_COMPILER_OPTION, ClassCompilerSelector.CompilerPolicy.JDK.name(), OptionScope.SESSION));


Hmmm...

We should not need every caller that sets a session option to say that it is setting a session option. Instead, the implementation of setOption for the session option manager class should fill in the OptionScope.SESSION value.

Fallback option manager actually implements setOption and session option manager which is a subclass of the fallback option managerdoesn't override that. Moreover, we are creating the option value and passing it. So we need to have a new method to set option in session option manager. Or can we have a variable "type" and set it to OptionType and set it ?

Look like the original API is lacking a nice, simple set(name, value) method... Let's leave that as a project for later.

paul-rogers · 2017-07-28T21:22:50Z

exec/java-exec/src/test/java/org/apache/drill/test/TestConfigLinkage.java

+  @Test
+  public void testConfigOption() throws Exception {
+    FixtureBuilder builder = ClusterFixture.builder()
+            .configProperty("drill.exec.options."+ExecConstants.SLICE_TARGET, 30);


Maybe add a new method to the fixture builder:

setOptionDefault(ExecConstants.SLICE_TARGET, Object value)

Then, inside this method, append the prefix. For the prefix, refer to a constant defined somewhere for the value.

paul-rogers · 2017-07-28T21:26:06Z

exec/java-exec/src/test/java/org/apache/drill/test/TestConfigLinkage.java

+      String affinity_factor = client.queryBuilder().sql("SELECT val FROM sys.options2 where name='planner.affinity_factor'").singletonString();
+      assertEquals(affinity_factor,"1.5");
+    }
+  }


Additional cases:

both session and system options set: the session option takes precedence

Query list of options using the new system table, with options in the four states (see below). Verify that the returned result is correct.

Various cases for the max width per node. (Will be tricky, you'll have to get the CPU count for use in verifying the values.)

Four states: default, system only, session only, both system and session.

Any other code paths that are not exercised by the above set of tests?

paul-rogers

Very nice work.
+1

ilooner · 2017-08-25T20:56:37Z

@dvjyothsna @paul-rogers Tim here. When do you guys think this PR will go in? Seems like it didn't make it into this last batch of commits :(. I will be opening another PR soon that is dependent on this going in.

paul-rogers · 2017-08-25T22:25:16Z

From a note sent by @dvjyothsna:

All the system/session option default values are externalized to the conf file. Now the options are looked up in the hierarchical order of SESSION - SYSTEM - CONFIG, the session being given the highest precedence. So, if the user doesn't set system/session option by issuing alter system/session, option manager fetches the value from the config. Now we need not bother about getting the value from config separately if it is not in the system/session level. For example:

OptionValue value = sessionOptions.getOption(JAVA_COMPILER_OPTION);
policy = CompilerPolicy.valueOf((value != null)
    ? value.string_val.toUpperCase()
    : config.getString(JAVA_COMPILER_CONFIG)

In the above code snippet we try to get option value and if it is not set (if it is null) we try to get it from Config. With my code changes if the value is not set at system/session level, value from the config is picked automatically.For example, the above can be simplified to:

policy = CompilerPolicy.valueOf(sessionOptions.getOption(JAVA_COMPILER_OPTION));

I have already externalized all the existing options in ExecConstants, PlannerSettings, etc.,
to the drill-module.conf. Following are the steps to add a new system/session option.

Steps to add a new system/session option:

Add a validator in the ExecConstants.java or PlannerSettings.java or whichever is appropriate for the option. For example:

           String NEW_OPTION_KEY = "drill.exec.new_option";
OptionValidator NEW_OPTION= new DoubleValidator(NEW_OPTION_KEY);

Add the validator from the above step to the validators list in SystemOptionManager.java.
Add the option in the conf file (for example, drill-module.conf or drill-override.conf file ) under the name space drill.exec.options. For example:

	drill.exec.options : {
		drill : {
			exec : {
				new_option : 1.5
			}
		}
	 }

Set the default value for the option in the conf file as shown above.
OptionManager loads default values from the conf file, User can override the default values by issuing ALTER SYSTEM/SESSION.

paul-rogers · 2017-08-26T03:54:55Z

@ilooner, @Ben-Zvi, the PR is now in master. Have fun!

closes apache#868

dvjyothsna closed this Jul 5, 2017

dvjyothsna reopened this Jul 5, 2017

dvjyothsna changed the title ~~Drill5547~~ Drill5547-Linking config options Jul 6, 2017

dvjyothsna changed the title ~~Drill5547-Linking config options~~ Drill-5547-Linking config options with system option manager Jul 6, 2017

dvjyothsna changed the title ~~Drill-5547-Linking config options with system option manager~~ Drill-5547:Linking config options with system option manager Jul 7, 2017

dvjyothsna changed the title ~~Drill-5547:Linking config options with system option manager~~ DRILL-5547:Linking config options with system option manager Jul 7, 2017

paul-rogers reviewed Jul 11, 2017

View reviewed changes

paul-rogers requested changes Jul 28, 2017

View reviewed changes

DRILL-5547:Linking config options with system option manager

f156dfa

dvjyothsna force-pushed the DRILL5547 branch from 0c2bf02 to f156dfa Compare August 17, 2017 18:59

paul-rogers approved these changes Aug 18, 2017

View reviewed changes

asfgit closed this in a51c98b Aug 26, 2017

weijietong pushed a commit to weijietong/drill that referenced this pull request Sep 14, 2017

DRILL-5547: Linking config options with system option manager

1454e81

closes apache#868

DRILL-5547:Linking config options with system option manager #868

DRILL-5547:Linking config options with system option manager #868

Uh oh!

Conversation

dvjyothsna commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paul-rogers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khurram-faraaz commented Jul 19, 2017

Uh oh!

paul-rogers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-rogers left a comment

Choose a reason for hiding this comment

Uh oh!

ilooner commented Aug 25, 2017

Uh oh!

paul-rogers commented Aug 25, 2017

Uh oh!

paul-rogers commented Aug 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dvjyothsna commented Jul 5, 2017 •

edited

Loading