[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/868 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r131735150
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java
---
@@ -68,7 +69,7 @@ public void testJaninoClassCompiler() throws Exception {
@Test
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));
--- End diff --
Look like the original API is lacking a nice, simple `set(name, value)`
method... Let's leave that as a project for later.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user dvjyothsna commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r131712468
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java
---
@@ -68,7 +69,7 @@ public void testJaninoClassCompiler() throws Exception {
@Test
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));
--- End diff --
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 ?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user dvjyothsna commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r131702178
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
---
@@ -144,4 +144,27 @@ public OptionList getOptionList() {
}
return list;
}
+
+ public OptionManager getFallback() {
+return fallback;
+ }
+
+ /**
+ * {@link FragmentOptionManager} and {@link SessionOptionManager} use
{@link SystemOptionManager} as the fall back
+ * manager so for both FragmentOptionManager and SessionOptionManager
fallback is the SystemOptionManager so it is
+ * returned. But in case of {@link QueryOptionManager}, it uses {@link
SessionOptionManager} as the fallback manager
+ * and since SessionOptionManager uses SystemOptionManager as fallback,
SystemOptionManager can be fetched from the
+ * SessionOptionManager.
+ */
+ public SystemOptionManager getSystemOptionManager() {
+final SystemOptionManager systemOptionManager;
+if(fallback instanceof SessionOptionManager) {
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130189636
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/test/TestConfigLinkage.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.drill.test;
+
+import org.apache.drill.exec.ExecConstants;
+import org.junit.Test;
+import org.apache.drill.common.config.DrillConfig;
+import static org.junit.Assert.assertEquals;
+/*
+ * Tests to test if the linkage between the two config option systems
+ * i.e., the linkage between boot-config system and system/session options.
+ * Tests to assert if the config options are read in the order of session
,system, boot-config */
+
+public class TestConfigLinkage {
+
+ /* Test if session option takes precendence */
+ @Test
+ public void testSessionOption() throws Exception {
+FixtureBuilder builder =
ClusterFixture.builder().sessionOption(ExecConstants.SLICE_TARGET, 10);
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+ String slice_target = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.slice_target'").singletonString();
+ assertEquals(slice_target,"10");
+}
+ }
+
+ /* Test if system option takes precendence */
+ @Test
+ public void testSystemOption() throws Exception {
+FixtureBuilder builder =
ClusterFixture.builder().systemOption(ExecConstants.SLICE_TARGET, 20);
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+ String slice_target = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.slice_target'").singletonString();
+ assertEquals(slice_target,"20");
+}
+ }
+
+ /* Test if config option takes precedence if config option is not set */
+ @Test
+ public void testConfigOption() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder()
+
.configProperty("drill.exec.options."+ExecConstants.SLICE_TARGET, 30);
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+ String slice_target = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.slice_target'").singletonString();
+ assertEquals(slice_target,"30");
+}
+ }
+
+ /* Test if altering system option takes precedence over config option */
+ @Test
+ public void testAlterSystem() throws Exception {
+try (ClusterFixture cluster = ClusterFixture.standardCluster();
+ ClientFixture client = cluster.clientFixture()) {
+ client.queryBuilder().sql("ALTER SYSTEM SET
`planner.affinity_factor` = 1.5").run();
+ String affinity_factor = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.affinity_factor'").singletonString();
+ assertEquals(affinity_factor,"1.5");
+}
+ }
--- End diff --
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?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r130187546 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ExtendedOptionIterator.java --- @@ -0,0 +1,142 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.store.sys; + +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + + +import com.google.common.collect.Lists; +import org.apache.drill.exec.ops.FragmentContext; +import org.apache.drill.exec.server.options.OptionManager; +import org.apache.drill.exec.server.options.OptionValue; +import org.apache.drill.exec.server.options.OptionValue.Kind; +import org.apache.drill.exec.server.options.OptionValue.OptionType; +import org.apache.drill.exec.server.options.OptionValue.OptionScope; + + +/* + * To extend the original Option iterator to hide the implementation details and to return the row + * which takes precedence over others for an option.This is done by examining the type as the precendence + * order is session - system - default.All the values are represented as String instead of having multiple + * columns and the datatype is provided for type details. --- End diff -- Maybe provide an example: foo, bar, mumble "fred", 10, SESSION With the "foo" etc. replaced with actual column names... --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130187202
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
---
@@ -217,19 +236,52 @@ public void validate(final OptionValue v, final
OptionSet manager) {
}
}
+ /** Max width is a special validator which computes and validates
+ * the maxwidth. If the maxwidth is already set in system/session
+ * the value is returned or else it is computed dynamically based on
+ * the available number of processors and cpu load average
+ */
+ public static class MaxWidthValidator extends TypeValidator{
+
+public MaxWidthValidator(String name) {
+ this(name, false);
+}
+
+public MaxWidthValidator(String name, boolean isAdminOption) {
+ super(name, Kind.LONG, isAdminOption);
+}
+
+public void loadDefault(DrillConfig bootConfig) {
+ OptionValue value = OptionValue.createLong(OptionType.SYSTEM,
getOptionName(), bootConfig.getLong(getConfigProperty()), OptionScope.BOOT);
+ setDefaultValue(value);
+}
+
+public int computeMaxWidth(double cpuLoadAverage, long maxWidth) {
+ // if maxwidth is already set return it
+ if (maxWidth != 0) {
+return (int) maxWidth;
+ }
+ // else compute the value and return
+ else {
+int maxWidthPerNode;
+int availProc = Runtime.getRuntime().availableProcessors();
+maxWidthPerNode = (int) Math.max(1, Math.min(availProc,
Math.round(availProc * cpuLoadAverage)));
--- End diff --
`int maxWidthPerNode = ...`
or even just
`return (int) ...`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r129929192
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java
---
@@ -106,4 +107,16 @@ public boolean isAdminOption() {
*/
public abstract Kind getKind();
+ /**
+ * Gets the default result option value for this validator.
+ *
+ * @return result default option value
+ */
+ public abstract void loadDefault(DrillConfig bootConfig);
+
+ /**
+ * Sets the default result option value for this validator.
+ */
+ public abstract void setDefaultValue(OptionValue defaultValue);
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r129928806 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java --- @@ -28,7 +29,7 @@ // the error messages produced by the validator private final String optionName; private final boolean isAdminOption; - + public static final String ConfigPath = "drill.exec.options."; --- End diff -- Java convention is all caps for constants: CONFIG_PATH. Since that name is somewhat generic, maybe OPTION_DEFAULTS_ROOT or some such. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r129929953
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
---
@@ -88,16 +101,17 @@ private OptionValue(@JsonProperty("kind") Kind kind,
@JsonProperty("num_val") Long num_val,
@JsonProperty("string_val") String string_val,
@JsonProperty("bool_val") Boolean bool_val,
- @JsonProperty("float_val") Double float_val) {
+ @JsonProperty("float_val") Double float_val,
+ @JsonProperty("scope") OptionScope scope) {
--- End diff --
The JSON version of this class is part of the drillbit-to-drillbit API.
Changing the definition means that Drillbits of one version can't interoperate
with Drillbits of another version. That is fine, we've never supported multiple
versions in a single cluster. But, there is interest in rolling upgrades which
would require such support. That is a separate task; just wanted to note the
compatibility issue here.
An alternative is to not serialize the scope. Serialization is used only to
send option values to fragments. But, at the fragment level, the code does not
care where the value was set. So, maybe keep the scope in this class, but don't
serialize it.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130188600
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java
---
@@ -68,7 +69,7 @@ public void testJaninoClassCompiler() throws Exception {
@Test
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));
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130188292
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -338,3 +338,142 @@ drill.exec: {
drill.jdbc: {
batch_queue_throttling_threshold: 100
}
+
+
--- End diff --
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."
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130189085
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/test/TestConfigLinkage.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.drill.test;
+
+import org.apache.drill.exec.ExecConstants;
+import org.junit.Test;
+import org.apache.drill.common.config.DrillConfig;
+import static org.junit.Assert.assertEquals;
+/*
+ * Tests to test if the linkage between the two config option systems
+ * i.e., the linkage between boot-config system and system/session options.
+ * Tests to assert if the config options are read in the order of session
,system, boot-config */
+
+public class TestConfigLinkage {
+
+ /* Test if session option takes precendence */
+ @Test
+ public void testSessionOption() throws Exception {
+FixtureBuilder builder =
ClusterFixture.builder().sessionOption(ExecConstants.SLICE_TARGET, 10);
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+ String slice_target = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.slice_target'").singletonString();
+ assertEquals(slice_target,"10");
+}
+ }
+
+ /* Test if system option takes precendence */
+ @Test
+ public void testSystemOption() throws Exception {
+FixtureBuilder builder =
ClusterFixture.builder().systemOption(ExecConstants.SLICE_TARGET, 20);
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+ String slice_target = client.queryBuilder().sql("SELECT val FROM
sys.options2 where name='planner.slice_target'").singletonString();
+ assertEquals(slice_target,"20");
+}
+ }
+
+ /* Test if config option takes precedence if config option is not set */
+ @Test
+ public void testConfigOption() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder()
+
.configProperty("drill.exec.options."+ExecConstants.SLICE_TARGET, 30);
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r129928166
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
---
@@ -144,4 +144,27 @@ public OptionList getOptionList() {
}
return list;
}
+
+ public OptionManager getFallback() {
+return fallback;
+ }
+
+ /**
+ * {@link FragmentOptionManager} and {@link SessionOptionManager} use
{@link SystemOptionManager} as the fall back
+ * manager so for both FragmentOptionManager and SessionOptionManager
fallback is the SystemOptionManager so it is
+ * returned. But in case of {@link QueryOptionManager}, it uses {@link
SessionOptionManager} as the fallback manager
+ * and since SessionOptionManager uses SystemOptionManager as fallback,
SystemOptionManager can be fetched from the
+ * SessionOptionManager.
+ */
+ public SystemOptionManager getSystemOptionManager() {
+final SystemOptionManager systemOptionManager;
+if(fallback instanceof SessionOptionManager) {
--- End diff --
A system option manager should never have a session option manager as a
fallback, right? Shouldn't it be the other way around?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130186974
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -314,6 +344,19 @@ public void deleteAllOptions(OptionType type) {
}
}
+ public void populateDefaultValues() {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
+for (OptionValidator validator: VALIDATORS.values()) {
+ try {
+ validator.loadDefault(bootConfig);
+ } catch (ConfigException.Missing e) {
+throw new IllegalStateException("Config Option is missing"+e+
validator.getOptionName());
--- End diff --
`...missing " + validator.getOptionName()`
The exception adds no real value here. Or:
`new IllegalStateException(..., e);`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130187872
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java
---
@@ -58,7 +58,10 @@ public static void
setupBufferedOpsMemoryAllocations(final PhysicalPlan plan, fi
// if there are any sorts, compute the maximum allocation, and set it
on them
if (bufferedOpList.size() > 0) {
final OptionManager optionManager = queryContext.getOptions();
- final long maxWidthPerNode =
optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val;
+ 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);
--- End diff --
Move declaration here: `final long maxWidthPerNode = ...`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r129928994
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java
---
@@ -106,4 +107,16 @@ public boolean isAdminOption() {
*/
public abstract Kind getKind();
+ /**
+ * Gets the default result option value for this validator.
--- End diff --
"default result option value" --> "default option value"
And below.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r130187797
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java
---
@@ -58,7 +58,10 @@ public static void
setupBufferedOpsMemoryAllocations(final PhysicalPlan plan, fi
// if there are any sorts, compute the maximum allocation, and set it
on them
if (bufferedOpList.size() > 0) {
final OptionManager optionManager = queryContext.getOptions();
- final long maxWidthPerNode =
optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val;
+ final long maxWidthPerNode;
+ double cpu_load_average =
optionManager.getOption(ExecConstants.CPU_LOAD_AVERAGE);
--- End diff --
`final double ...` and below.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r129928543 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionSet.java --- @@ -68,4 +68,14 @@ * @return the string value */ String getOption(TypeValidators.StringValidator validator); + + /** + * Gets the maxwidth value (from the option value) for the given validator. + * + * @param validator the maxwidth validator + * @return the maxwidth value + */ + long getOption(TypeValidators.MaxWidthValidator validator); --- End diff -- Needed? Can't this work if the MaxWidthValidator is an instance of a long validator? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126832672
--- 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
VALIDATORS) {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
--- End diff --
No need to create a new map. Setting the default value does not change the
key, so you can just update the fields of the value. (That is, we are not even
replacing the value; we are just altering a field in the value.)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126829118
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -201,9 +240,35 @@
public SystemOptionManager(LogicalPlanPersistence lpPersistence, final
PersistentStoreProvider provider) {
this.provider = provider;
-this.config =
PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(),
OptionValue.class)
-.name("sys.options")
-.build();
+this.config =
PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(),
OptionValue.class)
--- End diff --
A really handy trick to avoid code duplication is this:
```
public SystemOptionManager(LogicalPlanPersistence lpPersistence, final
PersistentStoreProvider provider) {
this(lpPersistence, provider, null);
}
```
Then, the bulk of the constructor code appears only once: in the three-arg
version.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126828646
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -48,11 +50,46 @@
* Only one instance of this class exists per drillbit. Options set at the
system level affect the entire system and
* persist between restarts.
*/
+
+/**
+ * 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
+ *
+ *
+ */
public class SystemOptionManager extends BaseOptionManager implements
OptionManager, AutoCloseable {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SystemOptionManager.class);
- private static final CaseInsensitiveMap VALIDATORS;
-
+ private static CaseInsensitiveMap VALIDATORS;
--- End diff --
Why no longer `final`? Once this map is built, it should never be replaced.
(The map itself can be changed; `final` just means that the `VALIDATORS`
variable itself can't be changed.)
And, see comments below in the method that does the defaults 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r126828426 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java --- @@ -48,11 +50,46 @@ * Only one instance of this class exists per drillbit. Options set at the system level affect the entire system and * persist between restarts. */ + +/** + * 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. + * --- End diff -- In Javadoc, `` is needed to separate paragraphs. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126824683
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java
---
@@ -72,7 +72,16 @@ public SimpleParallelizer(QueryContext context) {
OptionManager optionManager = context.getOptions();
long sliceTarget =
optionManager.getOption(ExecConstants.SLICE_TARGET).num_val;
this.parallelizationThreshold = sliceTarget > 0 ? sliceTarget : 1;
-this.maxWidthPerNode =
optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val.intValue();
+// 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);
+ this.maxWidthPerNode = (int)
Math.ceil(Runtime.getRuntime().availableProcessors() * cpu_load_average);
--- End diff --
this.maxWidthPerNode --> maxWidthPerNode
Here and below.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126834151
--- 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
VALIDATORS) {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
+for (final Map.Entry entry :
VALIDATORS.entrySet()) {
+
+ OptionValidator validator = entry.getValue();
+ final OptionValue.Kind kind = validator.getKind();
+ String name = entry.getValue().getOptionName();
+ OptionValue value;
+ String configPath = "drill.exec.options.";
+ String configName = configPath + name;
+ try {
+value = validator.loadConfigDefault(bootConfig,name,configPath);
+validator.setDefaultValue(value);
+tmp.put(name, validator);
+ } catch (ConfigException.Missing e) {
+logger.error(e.getMessage(), e);
--- End diff --
Seems we'd want to throw a fatal exception here, such as
`IllegalStateException` because this indicates, to the developers, that the
developer forgot to externalize the config default when they created the
validator.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126835015
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
---
@@ -139,6 +140,10 @@ public BooleanValidator(String name, boolean def) {
public BooleanValidator(String name, boolean def, boolean
isAdminOption) {
super(name, Kind.BOOLEAN,
OptionValue.createBoolean(OptionType.SYSTEM, name, def), isAdminOption);
}
+
+public OptionValue loadConfigDefault(DrillConfig bootConfig, String
name, String configPath){
+ return OptionValue.createBoolean(OptionType.SYSTEM, name,
bootConfig.getBoolean(configPath+name));
--- End diff --
Instead of `configPath+name` multiple times, if we turn `configPath` into a
constant, and get `name` from the validator itself, we want:
```
...getBoolean(getConfigProperty());
...
private String getConfigProperty() {
return CONFIG_PATH + getOptionName(); }
```
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126833990
--- 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
VALIDATORS) {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
+for (final Map.Entry entry :
VALIDATORS.entrySet()) {
+
+ OptionValidator validator = entry.getValue();
+ final OptionValue.Kind kind = validator.getKind();
+ String name = entry.getValue().getOptionName();
+ OptionValue value;
+ String configPath = "drill.exec.options.";
+ String configName = configPath + name;
+ try {
+value = validator.loadConfigDefault(bootConfig,name,configPath);
+validator.setDefaultValue(value);
+tmp.put(name, validator);
--- End diff --
As noted above, no need to alter the map as neither the key nor value
object changed.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r126828148 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java --- @@ -48,11 +50,46 @@ * Only one instance of this class exists per drillbit. Options set at the system level affect the entire system and * persist between restarts. */ + +/** + * Drill has two different config systems each with its own namespace.First being the HOCON based boot time config --- End diff -- namespace.First --> namespace. First (Insert space after period. Here and below.) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126833932
--- 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
VALIDATORS) {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
+for (final Map.Entry entry :
VALIDATORS.entrySet()) {
+
+ OptionValidator validator = entry.getValue();
+ final OptionValue.Kind kind = validator.getKind();
+ String name = entry.getValue().getOptionName();
+ OptionValue value;
+ String configPath = "drill.exec.options.";
+ String configName = configPath + name;
+ try {
+value = validator.loadConfigDefault(bootConfig,name,configPath);
+validator.setDefaultValue(value);
--- End diff --
Setting the default should be done in `loadConfigDefault`. Actually, since
there is only one place to load the default from, the name `loadDefault` is
fine.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126828954
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -201,9 +240,35 @@
public SystemOptionManager(LogicalPlanPersistence lpPersistence, final
PersistentStoreProvider provider) {
this.provider = provider;
-this.config =
PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(),
OptionValue.class)
-.name("sys.options")
-.build();
+this.config =
PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(),
OptionValue.class)
+ .name("sys.options")
+ .build();
+ }
+
+ public SystemOptionManager(LogicalPlanPersistence lpPersistence, final
PersistentStoreProvider provider, final DrillConfig bootConfig) {
+this.provider = provider;
+this.config =
PersistentStoreConfig.newJacksonBuilder(lpPersistence.getMapper(),
OptionValue.class)
+ .name("sys.options")
+ .build();
+this.bootConfig = bootConfig;
+populateDefualtValues(VALIDATORS);
--- End diff --
Defualt --> Default
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126835350
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ExtendedOptionIterator.java
---
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys;
+
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Arrays;
+
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.ops.FragmentContext;
+import org.apache.drill.exec.server.options.OptionManager;
+import org.apache.drill.exec.server.options.OptionValue;
+import org.apache.drill.exec.server.options.OptionValue.Kind;
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
+import org.apache.drill.exec.server.options.SystemOptionManager;
+/*
+ * To extend the original Option iterator to hide the implementation
details and to return the row
+ * which takes precedence over others for an option.This is done by
examining the type as the precendence
+ * order is session - system - default.All the values are represented as
String instead of having multiple
+ * columns and the datatype is provided for type details.
+ */
+public class ExtendedOptionIterator implements Iterator {
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(OptionIterator.class);
+
+ private final OptionManager fragmentOptions;
+ private final Iterator mergedOptions;
+
+ public ExtendedOptionIterator(FragmentContext context) {
+fragmentOptions = context.getOptions();
+final Iterator optionList;
+mergedOptions = sortOptions(fragmentOptions.iterator());
+ }
+ /*
+Sort options according to name and the type to remove the redundant
rows
+for the same option based on the type
+ */
+ public Iterator sortOptions(Iterator options)
+ {
+List values = Lists.newArrayList(options);
+List optionValues = Lists.newArrayList();
+List optionValueList = Lists.newArrayList();
+OptionValue value;
+OptionType type;
+OptionValue[] temp = new OptionValue[3];
+
+for (int i = 0; i < values.size(); i++){
+ value = values.get(i);
+ final OptionValue def =
SystemOptionManager.getValidator(value.name).getDefault();
+ if (value.equalsIgnoreType(def)) {
+type = OptionType.DEFAULT;
+
optionValueList.add(OptionValue.createOption(value.kind,type,value.name,value.getValue().toString()));
+ } else {
+type = value.type;
+
optionValueList.add(OptionValue.createOption(value.kind,type,value.name,value.getValue().toString()));
+ }
+}
+Collections.sort(optionValueList, new Comparator() {
+ @Override
+ public int compare(OptionValue v1, OptionValue v2) {
+int nameCmp = v1.name.compareTo(v2.name);
+if (nameCmp != 0) {
+ return nameCmp;
+}
+return v1.type.compareTo( v2.type);
+ }
+});
+
+for (int i = 0; i < optionValueList.size() ;i++ )
+{
+ value = optionValueList.get(i);
+ type = value.type ;
+ switch (type) {
+case DEFAULT:
+ temp[2] = value;
+ break;
+case SYSTEM:
+ temp[1] = value;
+case SESSION:
+ temp[0] = value;
+ break;
+ }
+ if(i == optionValueList.size() - 1 || (i < optionValueList.size()
&& !value.getName().equals(optionValueList.get(i+1).getName( {
+int j = 0;
+while (temp[j] == null) {
+ j++;
+}
+optionValues.add(temp[j]);
+Arrays.fill(temp,null);
+}
+}
+return optionValues.iterator();
+ }
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126836273
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -338,3 +338,127 @@ drill.exec: {
drill.jdbc: {
batch_queue_throttling_threshold: 100
}
+
+drill.exec.options: {
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126831642
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -271,12 +320,20 @@ public static OptionValidator getValidator(final
String name) {
public OptionValue getOption(final String name) {
// check local space (persistent store)
final OptionValue value = options.get(name.toLowerCase());
+OptionValue val;
if (value != null) {
return value;
}
-// otherwise, return default.
+// otherwise, return default set in the validator.
final OptionValidator validator = getValidator(name);
+if (validator.getDefault() != null) {
+ if
(!validator.getDefault().getValue().equals(validator.getDefault().getValue())) {
+System.out.println("Config and hardcoded values are" +
"\t" + validator.getDefault().getValue()
--- End diff --
We can't use println in production code. Instead, we can use the logger:
```
logger.trace("Config value: {}, hardcoded value: {},
validator.getDefault().getValue(),
validator.getDefault().getValue());
```
The logger will call `toString()` on each argument for us.
Actually, this code seems to be temporary debugging code: checking the
default value against hard-coded. Once the work is done, the hard-coded values
should no longer exist.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r126832436 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java --- @@ -48,11 +50,46 @@ * Only one instance of this class exists per drillbit. Options set at the system level affect the entire system and * persist between restarts. */ + +/** + * 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 --- End diff -- This description would be great as a comment for the pull request. Now, image that this code is committed to Drill, and someone reads this a year from now. By then, the comment will be describing ancient history. We can see that this kind of comment should describe the system the way it is (or will be) after this change without need to reference history. For those who might be familiar with the old way, we often point them to the JIRA that describes the change. Example: "Default for system properties are externalized to the boot-time config file. See DRILL-5547. ..." --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126825276
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java
---
@@ -72,7 +72,16 @@ public SimpleParallelizer(QueryContext context) {
OptionManager optionManager = context.getOptions();
long sliceTarget =
optionManager.getOption(ExecConstants.SLICE_TARGET).num_val;
this.parallelizationThreshold = sliceTarget > 0 ? sliceTarget : 1;
-this.maxWidthPerNode =
optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val.intValue();
+// 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);
+ this.maxWidthPerNode = (int)
Math.ceil(Runtime.getRuntime().availableProcessors() * cpu_load_average);
--- End diff --
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.)
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126836135
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java
---
@@ -58,7 +58,15 @@ public static void
setupBufferedOpsMemoryAllocations(final PhysicalPlan plan, fi
// if there are any sorts, compute the maximum allocation, and set it
on them
if (bufferedOpList.size() > 0) {
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);
--- End diff --
This code appears twice. Can we move it into a function so it appears once?
Maybe on to a purpose-build Validator class?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126824586
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java
---
@@ -72,7 +72,16 @@ public SimpleParallelizer(QueryContext context) {
OptionManager optionManager = context.getOptions();
long sliceTarget =
optionManager.getOption(ExecConstants.SLICE_TARGET).num_val;
this.parallelizationThreshold = sliceTarget > 0 ? sliceTarget : 1;
-this.maxWidthPerNode =
optionManager.getOption(ExecConstants.MAX_WIDTH_PER_NODE_KEY).num_val.intValue();
+// 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);
--- End diff --
Double --> double
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126836391
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -338,3 +338,127 @@ drill.exec: {
drill.jdbc: {
batch_queue_throttling_threshold: 100
}
+
+drill.exec.options: {
--- End diff --
Also, any chance we can alphabetize the list? Just run it through the
`sort` command? Will make future human access just a bit easier...
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126832838
--- 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
VALIDATORS) {
+
+// populate the options from the config
+final Map tmp = new HashMap<>();
+for (final Map.Entry entry :
VALIDATORS.entrySet()) {
+
+ OptionValidator validator = entry.getValue();
+ final OptionValue.Kind kind = validator.getKind();
+ String name = entry.getValue().getOptionName();
--- End diff --
Given that we get the name from the validator, we can iterate over map
values:
```
for (OptionValidator validator : validators.valueSet()) {
...
```
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_r126828858 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java --- @@ -193,6 +231,7 @@ private final PersistentStoreProvider provider; + private DrillConfig bootConfig = null; --- End diff -- ``` private final DrillConfig bootConfig; ``` This way, the member can be set exactly once in the constructor, which you are doing. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126831929
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -271,12 +320,20 @@ public static OptionValidator getValidator(final
String name) {
public OptionValue getOption(final String name) {
// check local space (persistent store)
final OptionValue value = options.get(name.toLowerCase());
+OptionValue val;
--- End diff --
Unused?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126828037
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
---
@@ -35,7 +36,7 @@
public class OptionValue implements Comparable {
public enum OptionType {
-BOOT, SYSTEM, SESSION, QUERY
+BOOT, DEFAULT, SYSTEM, SESSION, QUERY
--- End diff --
Actually, can't we reuse BOOT instead of adding DEFAULT?
As far as I can tell, the BOOT choice is used only in the options iterator
(DrillConfigIterator) to prepare a list for UI display. Since our defaults are
not BOOT options, can we just reuse that existing enum rather than having a
(config option for BOOT use vs. config option for DEFAULT use) confusion?
And, of course, this is a vestige of a deeper problem: the Type means "kind
of option: boot option, system-only option, or session option" to some code,
while it means "scope in which the option is actually set" to other code. Quite
a muddle!
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
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
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126835969
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/ExtendedOptionIterator.java
---
@@ -0,0 +1,152 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.sys;
+
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Arrays;
+
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.drill.exec.ops.FragmentContext;
+import org.apache.drill.exec.server.options.OptionManager;
+import org.apache.drill.exec.server.options.OptionValue;
+import org.apache.drill.exec.server.options.OptionValue.Kind;
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
+import org.apache.drill.exec.server.options.SystemOptionManager;
+/*
+ * To extend the original Option iterator to hide the implementation
details and to return the row
+ * which takes precedence over others for an option.This is done by
examining the type as the precendence
+ * order is session - system - default.All the values are represented as
String instead of having multiple
+ * columns and the datatype is provided for type details.
+ */
+public class ExtendedOptionIterator implements Iterator {
+// private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(OptionIterator.class);
+
+ private final OptionManager fragmentOptions;
+ private final Iterator mergedOptions;
+
+ public ExtendedOptionIterator(FragmentContext context) {
+fragmentOptions = context.getOptions();
+final Iterator optionList;
+mergedOptions = sortOptions(fragmentOptions.iterator());
+ }
+ /*
+Sort options according to name and the type to remove the redundant
rows
+for the same option based on the type
+ */
+ public Iterator sortOptions(Iterator options)
+ {
+List values = Lists.newArrayList(options);
+List optionValues = Lists.newArrayList();
+List optionValueList = Lists.newArrayList();
+OptionValue value;
+OptionType type;
+OptionValue[] temp = new OptionValue[3];
--- End diff --
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.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #868: DRILL-5547:Linking config options with system optio...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_r126827426
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java
---
@@ -106,4 +107,16 @@ public boolean isAdminOption() {
*/
public abstract Kind getKind();
+ /**
+ * Gets the default result option value for this validator.
+ *
+ * @return result default option value
+ */
+ public abstract OptionValue loadConfigDefault(DrillConfig bootConfig,
String name, String configPath);
--- End diff --
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...
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
