You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sohami <gi...@git.apache.org> on 2017/06/05 20:59:21 UTC
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
GitHub user sohami opened a pull request:
https://github.com/apache/drill/pull/849
DRILL-5568: Include hadoop-common jars inside drill-jdbc-all.jar
More details on this PR is in [JIRA](https://issues.apache.org/jira/browse/DRILL-5568)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sohami/drill DRILL-5568
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/849.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #849
----
commit e84ce5bb6317e7a8caa50c7ffc85dfc416616596
Author: Sorabh Hamirwasia <sh...@maprtech.com>
Date: 2017-06-05T20:45:27Z
DRILL-5568: Include hadoop-common jars inside drill-jdbc-all.jar
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122799104
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
--- End diff --
Maybe add a comment that the property is needed only when Hadoop classes are relocated. In a normal build, the property is not set and Hadoop classes are used normally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122796433
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
+ */
+ private void updateGroupMapping() {
+ final String originalClassName = get(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING);
+ final String profilePrefix = System.getProperty("namespacePrefix");
--- End diff --
Would recommend a name that is a bit less generic. Maybe "drill.security.namespacePrefix"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122847079
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillFactory.java ---
@@ -37,6 +38,24 @@
protected final int major;
protected final int minor;
+ static {
+ Properties prop = new Properties();
--- End diff --
Moved it to SecurityConfiguration.java. All unit tests are also passing and the application is also running fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122800022
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillFactory.java ---
@@ -37,6 +38,24 @@
protected final int major;
protected final int minor;
+ static {
+ Properties prop = new Properties();
--- End diff --
Actually, I think this code can be moved into `SecurityConfiguration.java`.
Java handles resources by looking in the class path, which is formed from all jars. A long as the resource file is somewhere in the top level of some jar, this code will find it. So, despite what I said in person the other day, the code need not be here to work.
That said, the property file DOES have to be in the JDBC-all package to avoid having multiple files of the same name in the class path.
Please try it out to see if the code works in `SecurityConfiguration.java`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/849
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122797316
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
+ */
+ private void updateGroupMapping() {
+ final String originalClassName = get(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING);
+ final String profilePrefix = System.getProperty("namespacePrefix");
+ final String updatedClassName = (profilePrefix != null) ? (profilePrefix + originalClassName)
+ : originalClassName;
+ set(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING, updatedClassName);
--- End diff --
```
if (! Strings.isNullOrEmpty(profilePrefix)) {
set(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING, profilePrefix.trim() + originalClassName);
}
```
?
Handles the case where the prefix is missing, or exists, but is empty. Or, perhaps I'm being overly cautions about stray spaces, empty properties, and not changing properties when not needed...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122799235
--- Diff: exec/jdbc-all/pom.xml ---
@@ -483,6 +518,269 @@
<profiles>
<profile>
+ <id>default</id>
--- End diff --
Not real happy about the amount of redundancy. But, I suppose Maven offers no alternative...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122847012
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
+ */
+ private void updateGroupMapping() {
+ final String originalClassName = get(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING);
+ final String profilePrefix = System.getProperty("namespacePrefix");
+ final String updatedClassName = (profilePrefix != null) ? (profilePrefix + originalClassName)
+ : originalClassName;
+ set(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING, updatedClassName);
--- End diff --
Changed to use` Strings.isNullOrEmpty`. I am already trimming the value of prefix befire setting it inside system property.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122846916
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #849: DRILL-5568: Include hadoop-common jars inside drill-jdbc-a...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/849
+ @paul-rogers - To help review this PR in case JIRA didn't sent out the email.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122847120
--- Diff: exec/jdbc-all/pom.xml ---
@@ -483,6 +518,269 @@
<profiles>
<profile>
+ <id>default</id>
--- End diff --
Yes didn't found any other way to set the default value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #849: DRILL-5568: Include hadoop-common jars inside drill...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/849#discussion_r122846935
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/security/SecurityConfiguration.java ---
@@ -0,0 +1,44 @@
+/*
+ * 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.rpc.security;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
+
+
+public class SecurityConfiguration extends Configuration {
+ //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SecurityConfiguration.class);
+
+ public SecurityConfiguration() {
+ super();
+ updateGroupMapping();
+ }
+
+ /**
+ * Update the Group Mapping class name to add namespace prefix retrieved from System Property. This is needed since
+ * in drill-jdbc-all jar we are packaging hadoop dependencies under that namespace. This will help application
+ * using this jar as driver to avoid conflict with it's own hadoop dependency if any.
+ */
+ private void updateGroupMapping() {
+ final String originalClassName = get(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING);
+ final String profilePrefix = System.getProperty("namespacePrefix");
--- End diff --
Updated
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---