You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Mohit Sabharwal <mo...@cloudera.com> on 2016/10/05 18:47:54 UTC
Re: Review Request 52283: HIVE-14822 : Add support for credential
provider for jobs launched from Hiveserver2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52283/#review151382
-----------------------------------------------------------
Took a quick pass. Have some questions and suggestions.
Will take a another pass once those are addressed.
common/src/java/org/apache/hadoop/hive/conf/Constants.java (lines 34 - 35)
<https://reviews.apache.org/r/52283/#comment219968>
Please provide some hint in the name that this is storing name of an environment variable.
For example: using ENVVAR instead of NAME
HIVE_SERVER2_JOB_CREDSTORE_PASSWORD_ENVVAR
HADOOP_CREDENTIAL_PASSWORD_ENVVAR
common/src/java/org/apache/hadoop/hive/conf/Constants.java (line 36)
<https://reviews.apache.org/r/52283/#comment219969>
Same. This is name of config and not the path, right ?
HADOOP_CREDENTIAL_PROVIDER_PATH_CONFIG
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2522)
<https://reviews.apache.org/r/52283/#comment219970>
Does this work for Spark ? Tez ? Good to explicitly add all engines that are known to support this functionality. If an engine has not been tested, I think it's ok to state that "this functionality has not been tested against <engine>".
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (lines 136 - 137)
<https://reviews.apache.org/r/52283/#comment219972>
Worth adding why environment variables are used instead of hive configs.
Since this is the main method could you summarize everything you are doing ?
For example:
If the driver is configured with HIVE_SERVER2_JOB_CREDSTORE_LOCATION,
AND
HIVE_JOB_CREDSTORE_PASSWORD environment is set then:
(1) JobConf.MAPRED_MAP_TASK_ENV should contain HIVE_SERVER2_JOB_CREDSTORE_LOCATION
(2) Set hadoop.security.credential.provider.path to HIVE_SERVER2_JOB_CREDSTORE_LOCATION
What do you mean by HIVE_JOB_CREDSTORE_PASSWORD
or HADOOP_CREDSTORE_PASSWORD ? You mean you'll take either one if set ? What if both are set ?
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 422)
<https://reviews.apache.org/r/52283/#comment219705>
Check if string is empty?
ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java (lines 35 - 36)
<https://reviews.apache.org/r/52283/#comment219706>
nit: please prefix with word "test"
nit: HIVE_JOB_CREDSTORE_PASSWORD_ENVVAR_VAL
HADOOP_CREDSTORE_PASSWORD_ENVVAR_VAL
ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java (lines 81 - 82)
<https://reviews.apache.org/r/52283/#comment219703>
I would use assertEquals here and get rid of the message. It's making these statements difficult to read without adding much value. If this assert fails, we know exactly what happened.
Same for all other instances in this file.
ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java (lines 88 - 94)
<https://reviews.apache.org/r/52283/#comment219975>
Are you testing the same thing twice ?
ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java (lines 109 - 112)
<https://reviews.apache.org/r/52283/#comment219977>
This is super unread-able... please make this into one line....
ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java (lines 333 - 335)
<https://reviews.apache.org/r/52283/#comment219974>
not need based on prior comment
- Mohit Sabharwal
On Sept. 30, 2016, 6:58 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2016, 6:58 p.m.)
>
>
> Review request for hive, Mohit Sabharwal and Sergio Pena.
>
>
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This change adds support for credential provider for jobs launched from HiveServer2. It also adds support for job-specific credential provider and password which is passed to the jobs via the job configs when they are launched from HS2. The hive specific credential provider location is specified by a new configuration property specific to hiveserver2 and the password to job credential provider is provided using the environment variable
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/EnvironmentUtils.java PRE-CREATION
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 3ed2d086fd8e14b9a70550c02082c1456f905a08
> common/src/java/org/apache/hadoop/hive/conf/Constants.java 77c6aa5663c2c5358715801bc039c0fe8035e3dc
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 43a16d7fed1d38400793e7c96a7febebe42d351b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 16c2eafdb6888ed62168dd00f76335fa2484753b
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java fd640567124e1bb8b558359732764a07c8b0f2ed
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java cea9582c8ccb0c79700ac7913735d4cdf52f0c7e
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 784b9c9fa769eeb088e3a6408a0b29147a8b4086
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java c75333d7022f776aab184a4b804fd7ad7befac64
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java a9f70c4100219c8929abd4e31b30d340e6ba98bd
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java PRE-CREATION
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 936fdafdb37c461a7a5deb97cba72d4db54a49e1
>
> Diff: https://reviews.apache.org/r/52283/diff/
>
>
> Testing
> -------
>
> Testing running multiple queries on S3 with keys stored in a credential provider
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
Re: Review Request 52283: HIVE-14822 : Add support for credential
provider for jobs launched from Hiveserver2
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
> On Oct. 5, 2016, 6:47 p.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java, line 422
> > <https://reviews.apache.org/r/52283/diff/3/?file=1516972#file1516972line422>
> >
> > Check if string is empty?
I wanted to revert back the original value of the credential provider once the job is submitted. checking empty is also not the right thing to do here. The check should be if it is not null we set it, but if it is null we need to unset it from the configuration. I modified it. Thanks for pointing it out.
> On Oct. 5, 2016, 6:47 p.m., Mohit Sabharwal wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java, lines 88-94
> > <https://reviews.apache.org/r/52283/diff/3/?file=1516976#file1516976line88>
> >
> > Are you testing the same thing twice ?
Its actually checkes against MAP task env and REDUCE task env. Hopefully it is more readable now after I made the changes you suggested.
- Vihang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52283/#review151382
-----------------------------------------------------------
On Oct. 5, 2016, 9:58 p.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2016, 9:58 p.m.)
>
>
> Review request for hive, Mohit Sabharwal and Sergio Pena.
>
>
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> This change adds support for credential provider for jobs launched from HiveServer2. It also adds support for job-specific credential provider and password which is passed to the jobs via the job configs when they are launched from HS2. The hive specific credential provider location is specified by a new configuration property specific to hiveserver2 and the password to job credential provider is provided using the environment variable
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/EnvironmentUtils.java PRE-CREATION
> common/src/java/org/apache/hadoop/hive/common/FileUtils.java 3ed2d086fd8e14b9a70550c02082c1456f905a08
> common/src/java/org/apache/hadoop/hive/conf/Constants.java 77c6aa5663c2c5358715801bc039c0fe8035e3dc
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 43a16d7fed1d38400793e7c96a7febebe42d351b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 16c2eafdb6888ed62168dd00f76335fa2484753b
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java fd640567124e1bb8b558359732764a07c8b0f2ed
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java cea9582c8ccb0c79700ac7913735d4cdf52f0c7e
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 784b9c9fa769eeb088e3a6408a0b29147a8b4086
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java c75333d7022f776aab184a4b804fd7ad7befac64
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java a9f70c4100219c8929abd4e31b30d340e6ba98bd
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java PRE-CREATION
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 936fdafdb37c461a7a5deb97cba72d4db54a49e1
>
> Diff: https://reviews.apache.org/r/52283/diff/
>
>
> Testing
> -------
>
> Testing running multiple queries on S3 with keys stored in a credential provider
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>