[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105709#comment-16105709
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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 = ...`
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105705#comment-16105705
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105710#comment-16105710
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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."
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[ https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105711#comment-16105711 ] ASF GitHub Bot commented on DRILL-5547: --- Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_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... > Drill config options and session options do not work as intended > > > Key: DRILL-5547 > URL: https://issues.apache.org/jira/browse/DRILL-5547 > Project: Apache Drill > Issue Type: Bug > Components: Server >Affects Versions: 1.10.0 >Reporter: Karthikeyan Manivannan >Assignee: Venkata Jyothsna Donapati > Fix For: Future > > > In Drill, session options should take precedence over config options. But > several of these session options are assigned hard-coded default values when > the option validators are initialized. Because of this config options will > never be read and honored even if the user did not specify the session > option. > ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as > the default value. This default value gets into the session options map via > the initialization of validators in SystemOptionManager. > Now any piece of code that tries to check if a session option is set will > never see a null, so it will always use that value and never try to look into > the config options. For example, in the following piece of code from > ClassCompilerSelector (), the policy will never be read from the config file. > policy = CompilerPolicy.valueOf((value != null) ? > value.string_val.toUpperCase() : > config.getString(JAVA_COMPILER_CONFIG).toUpperCase()); -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105701#comment-16105701
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy =
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[ https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105698#comment-16105698 ] ASF GitHub Bot commented on DRILL-5547: --- Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_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? > Drill config options and session options do not work as intended > > > Key: DRILL-5547 > URL: https://issues.apache.org/jira/browse/DRILL-5547 > Project: Apache Drill > Issue Type: Bug > Components: Server >Affects Versions: 1.10.0 >Reporter: Karthikeyan Manivannan >Assignee: Venkata Jyothsna Donapati > Fix For: Future > > > In Drill, session options should take precedence over config options. But > several of these session options are assigned hard-coded default values when > the option validators are initialized. Because of this config options will > never be read and honored even if the user did not specify the session > option. > ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as > the default value. This default value gets into the session options map via > the initialization of validators in SystemOptionManager. > Now any piece of code that tries to check if a session option is set will > never see a null, so it will always use that value and never try to look into > the config options. For example, in the following piece of code from > ClassCompilerSelector (), the policy will never be read from the config file. > policy = CompilerPolicy.valueOf((value != null) ? > value.string_val.toUpperCase() : > config.getString(JAVA_COMPILER_CONFIG).toUpperCase()); -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105707#comment-16105707
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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?
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105702#comment-16105702
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105699#comment-16105699
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105700#comment-16105700
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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?
> Drill config options and session options do not work as intended
>
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105703#comment-16105703
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105706#comment-16105706
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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);`
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105708#comment-16105708
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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) ...`
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[ https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105697#comment-16105697 ] ASF GitHub Bot commented on DRILL-5547: --- Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/868#discussion_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. > Drill config options and session options do not work as intended > > > Key: DRILL-5547 > URL: https://issues.apache.org/jira/browse/DRILL-5547 > Project: Apache Drill > Issue Type: Bug > Components: Server >Affects Versions: 1.10.0 >Reporter: Karthikeyan Manivannan >Assignee: Venkata Jyothsna Donapati > Fix For: Future > > > In Drill, session options should take precedence over config options. But > several of these session options are assigned hard-coded default values when > the option validators are initialized. Because of this config options will > never be read and honored even if the user did not specify the session > option. > ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as > the default value. This default value gets into the session options map via > the initialization of validators in SystemOptionManager. > Now any piece of code that tries to check if a session option is set will > never see a null, so it will always use that value and never try to look into > the config options. For example, in the following piece of code from > ClassCompilerSelector (), the policy will never be read from the config file. > policy = CompilerPolicy.valueOf((value != null) ? > value.string_val.toUpperCase() : > config.getString(JAVA_COMPILER_CONFIG).toUpperCase()); -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (DRILL-5547) Drill config options and session options do not work as intended
[
https://issues.apache.org/jira/browse/DRILL-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105704#comment-16105704
]
ASF GitHub Bot commented on DRILL-5547:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/868#discussion_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.
> Drill config options and session options do not work as intended
>
>
> Key: DRILL-5547
> URL: https://issues.apache.org/jira/browse/DRILL-5547
> Project: Apache Drill
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.10.0
>Reporter: Karthikeyan Manivannan
>Assignee: Venkata Jyothsna Donapati
> Fix For: Future
>
>
> In Drill, session options should take precedence over config options. But
> several of these session options are assigned hard-coded default values when
> the option validators are initialized. Because of this config options will
> never be read and honored even if the user did not specify the session
> option.
> ClassCompilerSelector.JAVA_COMPILER_VALIDATOR uses CompilerPolicy.DEFAULT as
> the default value. This default value gets into the session options map via
> the initialization of validators in SystemOptionManager.
> Now any piece of code that tries to check if a session option is set will
> never see a null, so it will always use that value and never try to look into
> the config options. For example, in the following piece of code from
> ClassCompilerSelector (), the policy will never be read from the config file.
> policy = CompilerPolicy.valueOf((value != null) ?
> value.string_val.toUpperCase() :
> config.getString(JAVA_COMPILER_CONFIG).toUpperCase());
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105652#comment-16105652
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130183909
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
--- End diff --
Good to give these names that suggest the test case:
`testMissingKeystorePath` for example.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105657#comment-16105657
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130183742
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
--- End diff --
`// Expected`
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105670#comment-16105670
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184855
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+ private final String keystorePath;
--- End diff --
We call this `keyStorePath` in the config, (upper-case the "S") so might as
well use the same convention here.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105668#comment-16105668
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130181769
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+ private final String keystorePath;
+
+ private final String keystorePassword;
+
+ private final String truststorePassword;
+
+ private final String truststorePath;
+
+ public boolean isValid = false;
+
+ public SSLConfig(Config config) throws DrillException {
+
+keystorePath =
config.getString(ExecConstants.HTTP_KEYSTORE_PATH).trim();
+
+keystorePassword =
config.getString(ExecConstants.HTTP_KEYSTORE_PASSWORD).trim();
+
+truststorePath =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PATH).trim();
+
+truststorePassword =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PASSWORD).trim();
+
+/*If keystorePath or keystorePassword is provided in the configuration
file use that*/
+if (!keystorePath.isEmpty() || !keystorePassword.isEmpty()) {
+ if (keystorePath.isEmpty() || keystorePassword.isEmpty()) {
+throw new DrillException(" *.ssl.keyStorePath and/or
*.ssl.keyStorePassword in the configuration file can't be empty.");
--- End diff --
For message, use the constant so the user has exact information:
```
...( ExecConstants.HTTP_KEYSTORE_PATH + " and/or " ...
```
Even better, help the user:
* Keystore path is provided, but password is missing.
* Password is provided, but keystore path is not set.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105658#comment-16105658
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130180741
--- Diff: distribution/src/resources/drill-override-example.conf ---
@@ -214,7 +214,7 @@ drill.exec: {
}
# Below SSL parameters need to be set for custom transport layer settings.
-javax.net.ssl {
+ssl{
--- End diff --
`ssl: {` under `drill.exec`
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105666#comment-16105666
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184027
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void secondTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void thirdTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ SSLConfig sslv = new SSLConfig(config);
+ assertEquals("root", sslv.getkeystorePassword());
+ assertEquals("/root", sslv.getkeystorePath());
+}
+ }
+}
+
--- End diff --
Delete extra lines
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105659#comment-16105659
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130183310
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -338,3 +338,11 @@ drill.exec: {
drill.jdbc: {
batch_queue_throttling_threshold: 100
}
+javax.net.ssl.keyStore = ""
+ssl.keyStorePath = ${?javax.net.ssl.keyStore}
+javax.net.ssl.keyStorePassword = ""
+ssl.keyStorePassword = ${?javax.net.ssl.keyStorePassword}
+javax.net.ssl.trustStore = ""
+ssl.trustStorePath = ${?javax.net.ssl.trustStore}
+javax.net.ssl.trustStorePassword = ""
+ssl.trustStorePassword = ${?javax.net.ssl.trustStorePassword}
--- End diff --
Add a comment about the `javax` versions being legacy and deprecated.
```
javax.net.ssl: {
keyStorePath = ...
...
}
drill.exec.ssl {
keyStorePath = "",
...
}
```
Better, move the `drill.exec` stuff under the existing `drill.exec` section.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105660#comment-16105660
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184346
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void secondTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void thirdTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
--- End diff --
Again, just a nit. Since all you need is the config, you can use the
`ConfigBuilder` directly:
```
DrillConfig config = new ConfigBuilder()
.put(...)
.build();
```
In this case, no need for the try/catch block and the operator fixture
since we are not using the memory allocator.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105669#comment-16105669
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184655
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+ private final String keystorePath;
+
+ private final String keystorePassword;
+
+ private final String truststorePassword;
+
+ private final String truststorePath;
+
+ public boolean isValid = false;
+
+ public SSLConfig(Config config) throws DrillException {
+
+keystorePath =
config.getString(ExecConstants.HTTP_KEYSTORE_PATH).trim();
+
+keystorePassword =
config.getString(ExecConstants.HTTP_KEYSTORE_PASSWORD).trim();
+
+truststorePath =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PATH).trim();
+
+truststorePassword =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PASSWORD).trim();
+
+/*If keystorePath or keystorePassword is provided in the configuration
file use that*/
+if (!keystorePath.isEmpty() || !keystorePassword.isEmpty()) {
+ if (keystorePath.isEmpty() || keystorePassword.isEmpty()) {
+throw new DrillException(" *.ssl.keyStorePath and/or
*.ssl.keyStorePassword in the configuration file can't be empty.");
+ }
+ isValid = true;
+}
+ }
+
+ public String getkeystorePath() {
+return keystorePath;
+ }
+
+ public String getkeystorePassword() {
+return keystorePassword;
+ }
+
+ public String gettruststorePath() {
+return truststorePath;
+ }
+
+ public String gettruststorePassword() {
--- End diff --
`getTrustStorePassword` That is, use `functionCase`... Here and above.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105651#comment-16105651
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130183662
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
--- End diff --
Not a big deal, but handy shortcut "fluent" style:
```
OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder()
.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "root")
.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
```
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105665#comment-16105665
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130181979
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -139,7 +142,13 @@ public void start() throws Exception {
final ServerConnector serverConnector;
if (config.getBoolean(ExecConstants.HTTP_ENABLE_SSL)) {
- serverConnector = createHttpsConnector();
+ try {
+serverConnector = createHttpsConnector();
+ }
+ catch(DrillException e){
+throw new DrillbitStartupException(e.getMessage(),e.getCause());
--- End diff --
`e.getCause()` --> `e` else you'll pass `null` to the startup exception.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105654#comment-16105654
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130182034
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -263,18 +272,17 @@ private ServerConnector createHttpsConnector() throws
Exception {
logger.info("Setting up HTTPS connector for web server");
final SslContextFactory sslContextFactory = new SslContextFactory();
+SSLConfig ssl = new SSLConfig(config);
-if (config.hasPath(ExecConstants.HTTP_KEYSTORE_PATH) &&
-
!Strings.isNullOrEmpty(config.getString(ExecConstants.HTTP_KEYSTORE_PATH))) {
+if(ssl.isValid){
--- End diff --
`if (ssl.isValid) {`
Here, `isValid` means `isSslEnabled`.
Better: `ssl.isEnabled()`
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105661#comment-16105661
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184900
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
--- End diff --
Not used, so can be deleted.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105667#comment-16105667
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130184493
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -0,0 +1,89 @@
+/**
+ * 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;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.SSLConfig;
+import org.apache.drill.test.OperatorFixture;
+import org.junit.Test;
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class TestSSLConfig {
+
+
+ @Test
+ public void firstTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void secondTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ try {
+SSLConfig sslv = new SSLConfig(config);
+fail();
+ } catch (Exception e) {
+assertTrue(e instanceof DrillException);
+ }
+}
+ }
+
+ @Test
+ public void thirdTest() throws Exception {
+
+OperatorFixture.OperatorFixtureBuilder builder =
OperatorFixture.builder();
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PASSWORD,
"root");
+builder.configBuilder().put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
+try (OperatorFixture fixture = builder.build()) {
+ DrillConfig config = fixture.config();
+ SSLConfig sslv = new SSLConfig(config);
+ assertEquals("root", sslv.getkeystorePassword());
+ assertEquals("/root", sslv.getkeystorePath());
+}
+ }
+}
--- End diff --
Do we need to test the trust store options? Are they subject to the same
"neither or both" constraints?
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[ https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105655#comment-16105655 ] ASF GitHub Bot commented on DRILL-5663: --- Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/874#discussion_r130181300 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java --- @@ -122,10 +122,10 @@ String HTTP_SESSION_MEMORY_RESERVATION = "drill.exec.http.session.memory.reservation"; String HTTP_SESSION_MEMORY_MAXIMUM = "drill.exec.http.session.memory.maximum"; String HTTP_SESSION_MAX_IDLE_SECS = "drill.exec.http.session_max_idle_secs"; - String HTTP_KEYSTORE_PATH = "javax.net.ssl.keyStore"; - String HTTP_KEYSTORE_PASSWORD = "javax.net.ssl.keyStorePassword"; - String HTTP_TRUSTSTORE_PATH = "javax.net.ssl.trustStore"; - String HTTP_TRUSTSTORE_PASSWORD = "javax.net.ssl.trustStorePassword"; + String HTTP_KEYSTORE_PATH = "ssl.keyStorePath"; --- End diff -- `drill.exec.ssl.keyStorePath` and below. > Drillbit fails to start when only keystore path is provided without keystore > password. > -- > > Key: DRILL-5663 > URL: https://issues.apache.org/jira/browse/DRILL-5663 > Project: Apache Drill > Issue Type: Bug >Reporter: Sorabh Hamirwasia >Assignee: Sindhuri Ramanarayan Rayavaram > > When we configure keystore path without keystore password inside > drill-override.conf for WebServer, then Drillbit fails to start. We should > explicitly check for either both being present or both being absent. If any > one of them is only present then throw startup exception for Drill. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105662#comment-16105662
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130182919
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -341,6 +349,7 @@ private ServerConnector createHttpsConnector() throws
Exception {
sslConnector.setPort(config.getInt(ExecConstants.HTTP_PORT));
return sslConnector;
+
--- End diff --
Remove blank line.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105664#comment-16105664
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130182809
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
---
@@ -263,18 +272,17 @@ private ServerConnector createHttpsConnector() throws
Exception {
logger.info("Setting up HTTPS connector for web server");
final SslContextFactory sslContextFactory = new SslContextFactory();
+SSLConfig ssl = new SSLConfig(config);
-if (config.hasPath(ExecConstants.HTTP_KEYSTORE_PATH) &&
-
!Strings.isNullOrEmpty(config.getString(ExecConstants.HTTP_KEYSTORE_PATH))) {
+if(ssl.isValid){
logger.info("Using configured SSL settings for web server");
-
sslContextFactory.setKeyStorePath(config.getString(ExecConstants.HTTP_KEYSTORE_PATH));
-
sslContextFactory.setKeyStorePassword(config.getString(ExecConstants.HTTP_KEYSTORE_PASSWORD));
-
- // TrustStore and TrustStore password are optional
- if (config.hasPath(ExecConstants.HTTP_TRUSTSTORE_PATH)) {
-
sslContextFactory.setTrustStorePath(config.getString(ExecConstants.HTTP_TRUSTSTORE_PATH));
-if (config.hasPath(ExecConstants.HTTP_TRUSTSTORE_PASSWORD)) {
-
sslContextFactory.setTrustStorePassword(config.getString(ExecConstants.HTTP_TRUSTSTORE_PASSWORD));
+
+ sslContextFactory.setKeyStorePath(ssl.getkeystorePath());
+ sslContextFactory.setKeyStorePassword(ssl.getkeystorePassword());
+ if(!ssl.gettruststorePath().isEmpty()){
--- End diff --
Better: `ssl.hasTrustStore()`
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105663#comment-16105663
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130182610
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+ private final String keystorePath;
+
+ private final String keystorePassword;
+
+ private final String truststorePassword;
+
+ private final String truststorePath;
+
+ public boolean isValid = false;
+
+ public SSLConfig(Config config) throws DrillException {
+
+keystorePath =
config.getString(ExecConstants.HTTP_KEYSTORE_PATH).trim();
+
+keystorePassword =
config.getString(ExecConstants.HTTP_KEYSTORE_PASSWORD).trim();
+
+truststorePath =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PATH).trim();
+
+truststorePassword =
config.getString(ExecConstants.HTTP_TRUSTSTORE_PASSWORD).trim();
+
+/*If keystorePath or keystorePassword is provided in the configuration
file use that*/
+if (!keystorePath.isEmpty() || !keystorePassword.isEmpty()) {
+ if (keystorePath.isEmpty() || keystorePassword.isEmpty()) {
+throw new DrillException(" *.ssl.keyStorePath and/or
*.ssl.keyStorePassword in the configuration file can't be empty.");
+ }
+ isValid = true;
--- End diff --
Confused by this one. Don't we have three cases?
* Neither property set. No SSL.
* Both properties set, use SSL.
* Incorrectly set, exception, this object is not created.
So `isValid` in the sense of "is the data valid" is unnecessary: the object
is not created if invalid. But, "isSslEnabled" is useful. Rather than exposing
a member variable, perhaps expose a function `isSslEnabled()`.
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105653#comment-16105653
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130180974
--- Diff: distribution/src/resources/drill-override-example.conf ---
@@ -214,7 +214,7 @@ drill.exec: {
}
# Below SSL parameters need to be set for custom transport layer settings.
-javax.net.ssl {
+ssl{
keyStore: "/keystore.file",
--- End diff --
This is an existing property. Generally, a leading slash means an absolute
path. So, do we expect the keystore to be in the Linux root directory? This
would be very unusual. Or, do we mean "keystore.file" relative to some implied
root directory? What is that root?
If we do not have a root, should we look relative to the $DRILL_CONF
directory (normally /usr/drill or $DRILL_HOME/conf or given by the --site
option.)
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5663) Drillbit fails to start when only keystore path is provided without keystore password.
[
https://issues.apache.org/jira/browse/DRILL-5663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105656#comment-16105656
]
ASF GitHub Bot commented on DRILL-5663:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/874#discussion_r130182211
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/SSLConfig.java
---
@@ -0,0 +1,70 @@
+/*
+ * 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;
+
+import com.typesafe.config.Config;
+import org.apache.drill.common.exceptions.DrillException;
+
+public class SSLConfig {
+ private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+ private final String keystorePath;
+
+ private final String keystorePassword;
+
+ private final String truststorePassword;
+
+ private final String truststorePath;
+
+ public boolean isValid = false;
--- End diff --
`final`
> Drillbit fails to start when only keystore path is provided without keystore
> password.
> --
>
> Key: DRILL-5663
> URL: https://issues.apache.org/jira/browse/DRILL-5663
> Project: Apache Drill
> Issue Type: Bug
>Reporter: Sorabh Hamirwasia
>Assignee: Sindhuri Ramanarayan Rayavaram
>
> When we configure keystore path without keystore password inside
> drill-override.conf for WebServer, then Drillbit fails to start. We should
> explicitly check for either both being present or both being absent. If any
> one of them is only present then throw startup exception for Drill.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16105624#comment-16105624
]
ASF GitHub Bot commented on DRILL-5660:
---
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/877#discussion_r130179724
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/MetadataVersion.java
---
@@ -0,0 +1,147 @@
+/*
+ * 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.parquet;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.ImmutableList;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+
+public class MetadataVersion implements Comparable {
+
+ private static final String FORMAT = "v((?!0)\\d+)\\.?((?!0)\\d+)?";
--- End diff --
To avoid having to parse the "v", might as well just say that the version
is a number. Define that we increment the major number (before the decimal
point) if the JSON structure changes, we increment the minor number (after the
decimal point) of the format is unchanged, but the semantics change (as in the
absolute/relative path change.) In general, convert the version to a float and
just do normal numeric comparison to determine if the file version is earlier,
same as, or later than the code version.
> Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
>
>
> Key: DRILL-5660
> URL: https://issues.apache.org/jira/browse/DRILL-5660
> Project: Apache Drill
> Issue Type: Bug
>Affects Versions: 1.11.0
>Reporter: Paul Rogers
>Assignee: Vitalii Diravka
> Labels: doc-impacting
> Fix For: 1.12.0
>
>
> Drill recently accepted a PR for the following bug:
> DRILL-3867: Store relative paths in metadata file
> This PR turned out to have a flaw which affects version compatibility.
> The DRILL-3867 PR changed the format of Parquet metadata files to store
> relative paths. All Drill servers after that PR create files with relative
> paths. But, the version number of the file is unchanged, so that older
> Drillbits don't know that they can't understand the file.
> Instead, if an older Drill tries to read the file, queries fail left and
> right. Drill will resolve the paths, but does so relative to the user's HDFS
> home directory, which is wrong.
> What should have happened is that we should have bumped the parquet metadata
> file version number so that older Drillbits can’t read the file. This ticket
> requests that we do that.
> Now, one could argue that the lack of version number change is fine. Once a
> user upgrades Drill, they won't use an old Drillbit. But, things are not that
> simple:
> * A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
> which metadata files have been created by a post-DRILL-3867 build. (This has
> already occurred multiple times in our shop.)
> * A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll
> back to Drill 1.10. Doing so will cause queries to fail due to
> seemingly-corrupt metadata files.
> * A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
> others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
> from the perspective of 1.10) and queries fail.
> Standard practice in this scenario is to:
> * Bump the file version number when the file format changes, and
> * Software refuses to read files with a version newer than the software was
> designed for.
> Of course, it is highly desirable that newer servers read old files, but that
> is not the issue here.
> *Main technical points of working of parquet metadata caching for now.*
> Only process
[jira] [Updated] (DRILL-5685) Provide a way to set common environment variable between sqlline and Drillbit differently.
[ https://issues.apache.org/jira/browse/DRILL-5685?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Sorabh Hamirwasia updated DRILL-5685: - Labels: ready-to-commit (was: ) > Provide a way to set common environment variable between sqlline and Drillbit > differently. > -- > > Key: DRILL-5685 > URL: https://issues.apache.org/jira/browse/DRILL-5685 > Project: Apache Drill > Issue Type: Task >Reporter: Sorabh Hamirwasia >Assignee: Sorabh Hamirwasia > Labels: ready-to-commit > Fix For: 1.12.0 > > > Drill has distrib-env.sh which is used to set any distribution specific > environment consumed by both sqlline and Drillbit. These environment > variables can be overridden by drill-env.sh. But there is no clean way to > know if these scripts were sourced for Drillbit or for sqlline, currently all > the variables will be set for both the scripts. > With this JIRA we will introduce a separated environment variable > _DRILLBIT_CONTEXT_ which will only be set inside drillbit.sh. Based on this > variable any script called later in the pipeline can make decision's to > set/unset an environment variable or to set the common environment variable > differently for sqlline and Drillbit. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16104819#comment-16104819
]
ASF GitHub Bot commented on DRILL-5660:
---
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/877#discussion_r130065442
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetMetadataCache.java
---
@@ -495,14 +495,14 @@ public void testFutureUnsupportedMetadataVersion()
throws Exception {
try {
test("use dfs_test.tmp");
test("create table `%s` as select * from cp.`tpch/nation.parquet`",
unsupportedMetadataVersion);
- String lastVersion =
Iterables.getLast(MetadataVersion.Constants.SUPPORTED_VERSIONS);
- for (String supportedVersion :
MetadataVersion.Constants.SUPPORTED_VERSIONS) {
-if (new MetadataVersion(lastVersion).compareTo(new
MetadataVersion(supportedVersion)) < 0) {
+ MetadataVersion lastVersion =
Iterables.getLast(MetadataVersion.Constants.SUPPORTED_VERSIONS);
+ for (MetadataVersion supportedVersion :
MetadataVersion.Constants.SUPPORTED_VERSIONS) {
+if (lastVersion.compareTo(supportedVersion) < 0) {
lastVersion = supportedVersion;
}
}
// Get the future version, which is absent in
MetadataVersion.SUPPORTED_VERSIONS list
- String futureVersion = "v" +
(Integer.parseInt(String.valueOf(lastVersion.charAt(1))) + 1);
+ String futureVersion = "v" +
(Integer.parseInt(String.valueOf(lastVersion.toString().charAt(1))) + 1);
--- End diff --
`String futureVersion = new MedataVersion(lastVersion.getMajor() + 1,
0).toString();`
Just add getters for major and minor versions to `MedataVersion` class
first.
> Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
>
>
> Key: DRILL-5660
> URL: https://issues.apache.org/jira/browse/DRILL-5660
> Project: Apache Drill
> Issue Type: Bug
>Affects Versions: 1.11.0
>Reporter: Paul Rogers
>Assignee: Vitalii Diravka
> Labels: doc-impacting
> Fix For: 1.12.0
>
>
> Drill recently accepted a PR for the following bug:
> DRILL-3867: Store relative paths in metadata file
> This PR turned out to have a flaw which affects version compatibility.
> The DRILL-3867 PR changed the format of Parquet metadata files to store
> relative paths. All Drill servers after that PR create files with relative
> paths. But, the version number of the file is unchanged, so that older
> Drillbits don't know that they can't understand the file.
> Instead, if an older Drill tries to read the file, queries fail left and
> right. Drill will resolve the paths, but does so relative to the user's HDFS
> home directory, which is wrong.
> What should have happened is that we should have bumped the parquet metadata
> file version number so that older Drillbits can’t read the file. This ticket
> requests that we do that.
> Now, one could argue that the lack of version number change is fine. Once a
> user upgrades Drill, they won't use an old Drillbit. But, things are not that
> simple:
> * A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
> which metadata files have been created by a post-DRILL-3867 build. (This has
> already occurred multiple times in our shop.)
> * A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll
> back to Drill 1.10. Doing so will cause queries to fail due to
> seemingly-corrupt metadata files.
> * A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
> others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
> from the perspective of 1.10) and queries fail.
> Standard practice in this scenario is to:
> * Bump the file version number when the file format changes, and
> * Software refuses to read files with a version newer than the software was
> designed for.
> Of course, it is highly desirable that newer servers read old files, but that
> is not the issue here.
> *Main technical points of working of parquet metadata caching for now.*
> Only process of reading the parquet metadata is changed (the process of
> writing isn't changed):
> +1. Metadata files are valid:+
> Metadata objects are created by deserialization of parquet metadata files in
> the process of creating ParquetGroupScan physical operator.
> All supported versions are stored in the "MetadataVersion.Constants" class
> and in the Jackson annotations for Metadata.ParquetTableMetadataBase class.
> +2. Metadata files version isn't supported (created by newer Drill version).
> Drill table has at least one metadata file of unsupported
[jira] [Commented] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16104821#comment-16104821
]
ASF GitHub Bot commented on DRILL-5660:
---
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/877#discussion_r130068040
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetMetadataCache.java
---
@@ -424,32 +432,121 @@ public void testMoveCache() throws Exception {
@Test
public void testMetadataCacheAbsolutePaths() throws Exception {
+final String absolutePathsMetadata = "absolute_paths_metadata";
try {
test("use dfs_test.tmp");
- final String relative_path_metadata_t1 = RELATIVE_PATHS_METADATA +
"/t1";
- final String relative_path_metadata_t2 = RELATIVE_PATHS_METADATA +
"/t2";
- test("create table `%s` as select * from cp.`tpch/nation.parquet`",
relative_path_metadata_t1);
- test("create table `%s` as select * from cp.`tpch/nation.parquet`",
relative_path_metadata_t2);
+ // creating two inner directories to leverage
METADATA_DIRECTORIES_FILENAME metadata file as well
+ final String absolutePathsMetadataT1 = absolutePathsMetadata + "/t1";
+ final String absolutePathsMetadataT2 = absolutePathsMetadata + "/t2";
+ test("create table `%s` as select * from cp.`tpch/nation.parquet`",
absolutePathsMetadataT1);
+ test("create table `%s` as select * from cp.`tpch/nation.parquet`",
absolutePathsMetadataT2);
copyMetaDataCacheToTempReplacingInternalPaths("parquet/metadata_with_absolute_path/"
+
- "metadata_directories_with_absolute_paths.requires_replace.txt",
RELATIVE_PATHS_METADATA, Metadata.METADATA_DIRECTORIES_FILENAME);
+ "metadata_directories_with_absolute_paths.requires_replace.txt",
absolutePathsMetadata, Metadata.METADATA_DIRECTORIES_FILENAME);
copyMetaDataCacheToTempReplacingInternalPaths("parquet/metadata_with_absolute_path/"
+
- "metadata_table_with_absolute_paths.requires_replace.txt",
RELATIVE_PATHS_METADATA, Metadata.METADATA_FILENAME);
+ "metadata_table_with_absolute_paths.requires_replace.txt",
absolutePathsMetadata, Metadata.METADATA_FILENAME);
copyMetaDataCacheToTempReplacingInternalPaths("parquet/metadata_with_absolute_path/"
+
- "metadata_table_with_absolute_paths_t1.requires_replace.txt",
relative_path_metadata_t1, Metadata.METADATA_FILENAME);
+ "metadata_table_with_absolute_paths_t1.requires_replace.txt",
absolutePathsMetadataT1, Metadata.METADATA_FILENAME);
copyMetaDataCacheToTempReplacingInternalPaths("parquet/metadata_with_absolute_path/"
+
- "metadata_table_with_absolute_paths_t2.requires_replace.txt",
relative_path_metadata_t2, Metadata.METADATA_FILENAME);
+ "metadata_table_with_absolute_paths_t2.requires_replace.txt",
absolutePathsMetadataT2, Metadata.METADATA_FILENAME);
+ String query = String.format("select * from %s",
absolutePathsMetadata);
+ int expectedRowCount = 50;
+ int expectedNumFiles = 1; // point to selectionRoot since no pruning
is done in this query
+ int actualRowCount = testSql(query);
+ assertEquals("An incorrect result was obtained while querying a
table with metadata cache files",
+ expectedRowCount, actualRowCount);
+ String numFilesPattern = "numFiles=" + expectedNumFiles;
+ String usedMetaPattern = "usedMetadataFile=true";
+ String cacheFileRootPattern = String.format("cacheFileRoot=%s/%s",
getDfsTestTmpSchemaLocation(), absolutePathsMetadata);
+ PlanTestBase.testPlanMatchingPatterns(query, new
String[]{numFilesPattern, usedMetaPattern, cacheFileRootPattern},
+ new String[] {"Filter"});
+} finally {
+ test("drop table if exists %s", absolutePathsMetadata);
+}
+ }
- int rowCount = testSql(String.format("select * from %s",
RELATIVE_PATHS_METADATA));
- assertEquals("An incorrect result was obtained while querying a
table with metadata cache files", 50, rowCount);
+ @Test
+ public void testSpacesInMetadataCachePath() throws Exception {
+final String pathWithSpaces = "path with spaces";
+try {
+ // creating multilevel table to store path with spaces in both
metadata files (METADATA and METADATA_DIRECTORIES)
+ test("create table dfs_test.tmp.`%s` as select * from
cp.`tpch/nation.parquet`", pathWithSpaces);
+ test("create table dfs_test.tmp.`%1$s/%1$s` as select * from
cp.`tpch/nation.parquet`", pathWithSpaces);
+ test("refresh table metadata dfs_test.tmp.`%s`", pathWithSpaces);
+ checkForMetadataFile(pathWithSpaces);
+ String query =
[jira] [Commented] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16104820#comment-16104820
]
ASF GitHub Bot commented on DRILL-5660:
---
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/877#discussion_r130067083
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetMetadataCache.java
---
@@ -495,14 +495,14 @@ public void testFutureUnsupportedMetadataVersion()
throws Exception {
try {
test("use dfs_test.tmp");
test("create table `%s` as select * from cp.`tpch/nation.parquet`",
unsupportedMetadataVersion);
- String lastVersion =
Iterables.getLast(MetadataVersion.Constants.SUPPORTED_VERSIONS);
- for (String supportedVersion :
MetadataVersion.Constants.SUPPORTED_VERSIONS) {
-if (new MetadataVersion(lastVersion).compareTo(new
MetadataVersion(supportedVersion)) < 0) {
+ MetadataVersion lastVersion =
Iterables.getLast(MetadataVersion.Constants.SUPPORTED_VERSIONS);
--- End diff --
No need to this manually:
```
List supportedVersions = new
ArrayList(MetadataVersion.Constants.SUPPORTED_VERSIONS);
Collections.sort(supportedVersions);
MetadataVersion lastVersion =
supportedVersions.get(supportedVersions.size() - 1);
```
> Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
>
>
> Key: DRILL-5660
> URL: https://issues.apache.org/jira/browse/DRILL-5660
> Project: Apache Drill
> Issue Type: Bug
>Affects Versions: 1.11.0
>Reporter: Paul Rogers
>Assignee: Vitalii Diravka
> Labels: doc-impacting
> Fix For: 1.12.0
>
>
> Drill recently accepted a PR for the following bug:
> DRILL-3867: Store relative paths in metadata file
> This PR turned out to have a flaw which affects version compatibility.
> The DRILL-3867 PR changed the format of Parquet metadata files to store
> relative paths. All Drill servers after that PR create files with relative
> paths. But, the version number of the file is unchanged, so that older
> Drillbits don't know that they can't understand the file.
> Instead, if an older Drill tries to read the file, queries fail left and
> right. Drill will resolve the paths, but does so relative to the user's HDFS
> home directory, which is wrong.
> What should have happened is that we should have bumped the parquet metadata
> file version number so that older Drillbits can’t read the file. This ticket
> requests that we do that.
> Now, one could argue that the lack of version number change is fine. Once a
> user upgrades Drill, they won't use an old Drillbit. But, things are not that
> simple:
> * A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
> which metadata files have been created by a post-DRILL-3867 build. (This has
> already occurred multiple times in our shop.)
> * A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll
> back to Drill 1.10. Doing so will cause queries to fail due to
> seemingly-corrupt metadata files.
> * A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
> others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
> from the perspective of 1.10) and queries fail.
> Standard practice in this scenario is to:
> * Bump the file version number when the file format changes, and
> * Software refuses to read files with a version newer than the software was
> designed for.
> Of course, it is highly desirable that newer servers read old files, but that
> is not the issue here.
> *Main technical points of working of parquet metadata caching for now.*
> Only process of reading the parquet metadata is changed (the process of
> writing isn't changed):
> +1. Metadata files are valid:+
> Metadata objects are created by deserialization of parquet metadata files in
> the process of creating ParquetGroupScan physical operator.
> All supported versions are stored in the "MetadataVersion.Constants" class
> and in the Jackson annotations for Metadata.ParquetTableMetadataBase class.
> +2. Metadata files version isn't supported (created by newer Drill version).
> Drill table has at least one metadata file of unsupported version:+
> JsonMappingException is obtained and swallowed without creating metadata
> object. Error message is logged. The state is stored in MetadataContext,
> therefore further there will be no attempt to deserialize metadata file again
> in context of performing current query. The physical plan will be created
> without using parquet metadata caching. Warning message is logged for every
> further check "is metadata corrupted".
> +3. Drill table has at least
[jira] [Updated] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Arina Ielchiieva updated DRILL-5660:
Fix Version/s: 1.12.0
> Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
>
>
> Key: DRILL-5660
> URL: https://issues.apache.org/jira/browse/DRILL-5660
> Project: Apache Drill
> Issue Type: Bug
>Affects Versions: 1.11.0
>Reporter: Paul Rogers
>Assignee: Vitalii Diravka
> Labels: doc-impacting
> Fix For: 1.12.0
>
>
> Drill recently accepted a PR for the following bug:
> DRILL-3867: Store relative paths in metadata file
> This PR turned out to have a flaw which affects version compatibility.
> The DRILL-3867 PR changed the format of Parquet metadata files to store
> relative paths. All Drill servers after that PR create files with relative
> paths. But, the version number of the file is unchanged, so that older
> Drillbits don't know that they can't understand the file.
> Instead, if an older Drill tries to read the file, queries fail left and
> right. Drill will resolve the paths, but does so relative to the user's HDFS
> home directory, which is wrong.
> What should have happened is that we should have bumped the parquet metadata
> file version number so that older Drillbits can’t read the file. This ticket
> requests that we do that.
> Now, one could argue that the lack of version number change is fine. Once a
> user upgrades Drill, they won't use an old Drillbit. But, things are not that
> simple:
> * A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
> which metadata files have been created by a post-DRILL-3867 build. (This has
> already occurred multiple times in our shop.)
> * A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll
> back to Drill 1.10. Doing so will cause queries to fail due to
> seemingly-corrupt metadata files.
> * A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
> others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
> from the perspective of 1.10) and queries fail.
> Standard practice in this scenario is to:
> * Bump the file version number when the file format changes, and
> * Software refuses to read files with a version newer than the software was
> designed for.
> Of course, it is highly desirable that newer servers read old files, but that
> is not the issue here.
> *Main technical points of working of parquet metadata caching for now.*
> Only process of reading the parquet metadata is changed (the process of
> writing isn't changed):
> +1. Metadata files are valid:+
> Metadata objects are created by deserialization of parquet metadata files in
> the process of creating ParquetGroupScan physical operator.
> All supported versions are stored in the "MetadataVersion.Constants" class
> and in the Jackson annotations for Metadata.ParquetTableMetadataBase class.
> +2. Metadata files version isn't supported (created by newer Drill version).
> Drill table has at least one metadata file of unsupported version:+
> JsonMappingException is obtained and swallowed without creating metadata
> object. Error message is logged. The state is stored in MetadataContext,
> therefore further there will be no attempt to deserialize metadata file again
> in context of performing current query. The physical plan will be created
> without using parquet metadata caching. Warning message is logged for every
> further check "is metadata corrupted".
> +3. Drill table has at least one corrupted metadata file, which can't be
> deserialized:+
> JsonParseException is obtained. Then the same behaviour as for the
> unsupported version files.
> +4. The metadata file was removed by other process:+
> FileNotFound is obtained. Then the same behaviour as for the unsupported
> version files.
> The new versions of metadata should be added in such manner:
> 1. Increasing of the metadata major version if metadata structure is changed.
> 2. Increasing of the metadata minor version if only metadata content is
> changed, but metadata structure is the same.
> For the first case a new metadata structure (class) should be created
> (possible an improvement of deserializing metadata files of any version into
> one strucure by using special converting)
> For the second case only annotation for the last metadata structure can be
> updated.
> *Summary*
> 1. Drill will read and use metadata files if files are valid, all present and
> supported. Under supported we mean that files were created before and under
> current Drill version.
> 2. Drill will ignore reading metadata files if at least one file is missing,
> empty, corrupted or unsupported. Under unsupported we mean files created
> after
[jira] [Updated] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Arina Ielchiieva updated DRILL-5660:
Description:
Drill recently accepted a PR for the following bug:
DRILL-3867: Store relative paths in metadata file
This PR turned out to have a flaw which affects version compatibility.
The DRILL-3867 PR changed the format of Parquet metadata files to store
relative paths. All Drill servers after that PR create files with relative
paths. But, the version number of the file is unchanged, so that older
Drillbits don't know that they can't understand the file.
Instead, if an older Drill tries to read the file, queries fail left and right.
Drill will resolve the paths, but does so relative to the user's HDFS home
directory, which is wrong.
What should have happened is that we should have bumped the parquet metadata
file version number so that older Drillbits can’t read the file. This ticket
requests that we do that.
Now, one could argue that the lack of version number change is fine. Once a
user upgrades Drill, they won't use an old Drillbit. But, things are not that
simple:
* A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
which metadata files have been created by a post-DRILL-3867 build. (This has
already occurred multiple times in our shop.)
* A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll back
to Drill 1.10. Doing so will cause queries to fail due to seemingly-corrupt
metadata files.
* A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
from the perspective of 1.10) and queries fail.
Standard practice in this scenario is to:
* Bump the file version number when the file format changes, and
* Software refuses to read files with a version newer than the software was
designed for.
Of course, it is highly desirable that newer servers read old files, but that
is not the issue here.
*Main technical points of working of parquet metadata caching for now.*
Only process of reading the parquet metadata is changed (the process of writing
isn't changed):
+1. Metadata files are valid:+
Metadata objects are created by deserialization of parquet metadata files in
the process of creating ParquetGroupScan physical operator.
All supported versions are stored in the "MetadataVersion.Constants" class and
in the Jackson annotations for Metadata.ParquetTableMetadataBase class.
+2. Metadata files version isn't supported (created by newer Drill version).
Drill table has at least one metadata file of unsupported version:+
JsonMappingException is obtained and swallowed without creating metadata
object. Error message is logged. The state is stored in MetadataContext,
therefore further there will be no attempt to deserialize metadata file again
in context of performing current query. The physical plan will be created
without using parquet metadata caching. Warning message is logged for every
further check "is metadata corrupted".
+3. Drill table has at least one corrupted metadata file, which can't be
deserialized:+
JsonParseException is obtained. Then the same behaviour as for the unsupported
version files.
+4. The metadata file was removed by other process:+
FileNotFound is obtained. Then the same behaviour as for the unsupported
version files.
The new versions of metadata should be added in such manner:
1. Increasing of the metadata major version if metadata structure is changed.
2. Increasing of the metadata minor version if only metadata content is
changed, but metadata structure is the same.
For the first case a new metadata structure (class) should be created (possible
an improvement of deserializing metadata files of any version into one strucure
by using special converting)
For the second case only annotation for the last metadata structure can be
updated.
*Summary*
1. Drill will read and use metadata files if files are valid, all present and
supported. Under supported we mean that files were created before and under
current Drill version.
2. Drill will ignore reading metadata files if at least one file is missing,
empty, corrupted or unsupported. Under unsupported we mean files created after
current Drill version. When metadata files are not used, warning message will
be written to the logs.
was:
Drill recently accepted a PR for the following bug:
DRILL-3867: Store relative paths in metadata file
This PR turned out to have a flaw which affects version compatibility.
The DRILL-3867 PR changed the format of Parquet metadata files to store
relative paths. All Drill servers after that PR create files with relative
paths. But, the version number of the file is unchanged, so that older
Drillbits don't know that they can't understand the file.
Instead, if an older Drill tries to read the file, queries fail left
[jira] [Updated] (DRILL-5660) Drill 1.10 queries fail due to Parquet Metadata "corruption" from DRILL-3867
[
https://issues.apache.org/jira/browse/DRILL-5660?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Arina Ielchiieva updated DRILL-5660:
Description:
Drill recently accepted a PR for the following bug:
DRILL-3867: Store relative paths in metadata file
This PR turned out to have a flaw which affects version compatibility.
The DRILL-3867 PR changed the format of Parquet metadata files to store
relative paths. All Drill servers after that PR create files with relative
paths. But, the version number of the file is unchanged, so that older
Drillbits don't know that they can't understand the file.
Instead, if an older Drill tries to read the file, queries fail left and right.
Drill will resolve the paths, but does so relative to the user's HDFS home
directory, which is wrong.
What should have happened is that we should have bumped the parquet metadata
file version number so that older Drillbits can’t read the file. This ticket
requests that we do that.
Now, one could argue that the lack of version number change is fine. Once a
user upgrades Drill, they won't use an old Drillbit. But, things are not that
simple:
* A developer tests a branch based on a pre-DRILL-3867 build on a cluster in
which metadata files have been created by a post-DRILL-3867 build. (This has
already occurred multiple times in our shop.)
* A user tries to upgrade to Drill 1.11, finds an issue, and needs to roll back
to Drill 1.10. Doing so will cause queries to fail due to seemingly-corrupt
metadata files.
* A user tries to do a rolling upgrade: running 1.11 on some servers, 1.10 on
others. Once a 1.11 server is installed, the metadata is updated ("corrupted"
from the perspective of 1.10) and queries fail.
Standard practice in this scenario is to:
* Bump the file version number when the file format changes, and
* Software refuses to read files with a version newer than the software was
designed for.
Of course, it is highly desirable that newer servers read old files, but that
is not the issue here.
*Main technical points of working of parquet metadata caching for now.*
Only process of reading the parquet metadata is changed (the process of writing
isn't changed):
+1. Metadata files are valid:+
Metadata objects are created by deserialization of parquet metadata files in
the process of creating ParquetGroupScan physical operator.
All supported versions are stored in the "MetadataVersion.Constants" class and
in the Jackson annotations for Metadata.ParquetTableMetadataBase class.
+2. Metadata files version isn't supported (created by newer Drill version).
Drill table has at least one metadata file of unsupported version:+
JsonMappingException is obtained and swallowed without creating metadata
object. Error message is logged. The state is stored in MetadataContext,
therefore further there will be no attempt to deserialize metadata file again
in context of performing current query. The physical plan will be created
without using parquet metadata caching. Warning message is logged for every
further check "is metadata corrupted".
+3. Drill table has at least one corrupted metadata file, which can't be
deserialized:+
JsonParseException is obtained. Then the same behaviour as for the unsupported
version files.
+4. The metadata file was removed by other process:+
FileNotFound is obtained. Then the same behaviour as for the unsupported
version files.
The new versions of metadata should be added in such manner:
1. Increasing of the metadata major version if metadata structure is changed.
2. Increasing of the metadata minor version if only metadata content is
changed, but metadata structure is the same.
For the first case a new metadata structure (class) should be created (possible
an improvement of deserializing metadata files of any version into one strucure
by using special converting)
For the second case only annotation for the last metadata structure can be
updated.
*Summary*
1. Drill will read and use metadata files if files are valid, all present and
supported. Under supported we mean that files were created before and under
current Drill version.
2. Drill will ignore reading metadata files if at least one files is missing,
empty, corrupted or unsupported. Under unsupported we mean files created after
current Drill version. When files are not used, warning message will be written
to the logs.
was:
Drill recently accepted a PR for the following bug:
DRILL-3867: Store relative paths in metadata file
This PR turned out to have a flaw which affects version compatibility.
The DRILL-3867 PR changed the format of Parquet metadata files to store
relative paths. All Drill servers after that PR create files with relative
paths. But, the version number of the file is unchanged, so that older
Drillbits don't know that they can't understand the file.
Instead, if an older Drill tries to read the file, queries fail left and
