You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by cheng xu <ch...@intel.com> on 2014/11/20 15:19:54 UTC
Review Request 28283: HIVE-8900:Create encryption testing framework
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs
-----
data/conf/encrypted/hive-site.xml PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review62361
-----------------------------------------------------------
data/conf/encrypted/hive-site.xml
<https://reviews.apache.org/r/28283/#comment104410>
Just to confirm the value of this variable. This value is the directory name for the staging dir that will be created like this:
i.e (value '.stagindir')
/user/hive/warehouse/table/.stagingdir
The value is not an absolute path to where the temporary files will be stored, but relative to the table location.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment104411>
This API (as weel as DFSConfigKeys) does not exist on older versions of hadoop (<= 2.6.0). We should make QTestUtil compatible with older versions.
Should we use a hadoop shim for this?
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment104412>
Should we use hadoop shims to get the key provider? Just to make QTestUtil compatible with older versions of hadoop.
- Sergio Pena
On Nov. 20, 2014, 2:19 p.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 2:19 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> data/conf/encrypted/hive-site.xml PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> >
Thanks for your review. Please see my inline comments.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 268
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line268>
> >
> > Do we need to set this value? For what I know, AES/CTR/NoPadding is the only cipher mode that HDFS supports.
Yes, you are right. We can remove it at this point. I add the setter here just in case one or more cipher will be supported later.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 365
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line365>
> >
> > I think this method 'in itEncryptionRelatedConfIfNeeded()' can be called inside the block line 370
> > as it is only called when clusterType is encrypted. Also, we may rename the method for a shorter name as IfNeeded won't be used.
I am afriad not since the initialization of dfs needs the security related properties. To clean the code, I do a change in this snippet.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 372
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line372>
> >
> > What if we move this line inside initEncryptionConf()? It is part of encryption initialization.
What the initEncryptionConf did is trying to set the security related properties. Another bigger consideration is that the fs needs the security related configuration and we have to complete the configuration setting work before the initilazing dfs or hes.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 754
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line754>
> >
> > - May we rename this method so that starts with the 'init' verb? This is just a good pratice I've learned in order
> > to read code much better. Also, IfNeeded() is the correct syntax.
> > - We could also get rid of the IfNeeded() word (making the name shorter) if if add the validation when this method
> > is called instead of inside the method. It is just an opinion.
Thanks for your suggestion. FIXED it.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 785
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line785>
> >
> > Just to comment that AES-256 can be used only if JCE is installed in your environment. Otherwise, any encryption
> > with this key will fail. Keys can be created, but when you try to encrypt something, fails. We should put a
> > comment here so that another developer knows this.
FIXED
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java, line 872
> > <https://reviews.apache.org/r/28283/diff/3/?file=778009#file778009line872>
> >
> > I think we should leave the 'hadoop.encryption.is.not.supported' key name on unsupported hadoop versions. This was left only as a comment for developers. Nobody will use this configuration key anyways.
FIXED
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, line 497
> > <https://reviews.apache.org/r/28283/diff/3/?file=778010#file778010line497>
> >
> > I think we should leave the 'hadoop.encryption.is.not.supported' key name on unsupported hadoop versions. This was left only as a comment for developers. Nobody will use this configuration key anyways.
FIXED
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, lines 498-499
> > <https://reviews.apache.org/r/28283/diff/3/?file=778010#file778010line498>
> >
> > Do we need these two configuration values in the configuration environment? These are used only for test purposes on QTestUtil. The user won't use these fields on hive-site.xml ever. Or not yet.
FIXED
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java, lines 960-970
> > <https://reviews.apache.org/r/28283/diff/3/?file=778011#file778011line960>
> >
> > Why was this block removed? I see the keyProvider variable is initialized inside getMiniDfs() method (testing). But what will happen with production code?
We should get the key provider via the name node who has already created a key provider. It has no different for the KMS while not for the java key provider. For java key provider, it stores the key into a file. And I digged into the code and found that two key providers are trying to open the same key file and one thread will not be awared of the changed key by another thread.
> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > ql/src/test/templates/TestEncrytedHDFSCliDriver.vm, line 1
> > <https://reviews.apache.org/r/28283/diff/3/?file=778008#file778008line1>
> >
> > Why do we need this new class instead of TestCliDriver.vm?
FIXED
- cheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------
On Nov. 28, 2014, 1:45 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2014, 1:45 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> ql/src/test/templates/TestEncrytedHDFSCliDriver.vm PRE-CREATION
> shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java ff7a82c
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
> On Dic. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 268
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line268>
> >
> > Do we need to set this value? For what I know, AES/CTR/NoPadding is the only cipher mode that HDFS supports.
>
> cheng xu wrote:
> Yes, you are right. We can remove it at this point. I add the setter here just in case one or more cipher will be supported later.
Thanks. I don't think hdfs will add new ciphers in the near future. CTR is the only cipher mode that fits with how files are stored on hdfs, and it is very secure, and AES is the best approved algorithm by PCI & HIPAA these days.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------
On Dic. 2, 2014, 2:58 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dic. 2, 2014, 2:58 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105686>
Do we need to set this value? For what I know, AES/CTR/NoPadding is the only cipher mode that HDFS supports.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105687>
I think this method 'in itEncryptionRelatedConfIfNeeded()' can be called inside the block line 370
as it is only called when clusterType is encrypted. Also, we may rename the method for a shorter name as IfNeeded won't be used.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105688>
What if we move this line inside initEncryptionConf()? It is part of encryption initialization.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105689>
- May we rename this method so that starts with the 'init' verb? This is just a good pratice I've learned in order
to read code much better. Also, IfNeeded() is the correct syntax.
- We could also get rid of the IfNeeded() word (making the name shorter) if if add the validation when this method
is called instead of inside the method. It is just an opinion.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105690>
Just to comment that AES-256 can be used only if JCE is installed in your environment. Otherwise, any encryption
with this key will fail. Keys can be created, but when you try to encrypt something, fails. We should put a
comment here so that another developer knows this.
ql/src/test/templates/TestEncrytedHDFSCliDriver.vm
<https://reviews.apache.org/r/28283/#comment105692>
Why do we need this new class instead of TestCliDriver.vm?
shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java
<https://reviews.apache.org/r/28283/#comment105696>
I think we should leave the 'hadoop.encryption.is.not.supported' key name on unsupported hadoop versions. This was left only as a comment for developers. Nobody will use this configuration key anyways.
shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java
<https://reviews.apache.org/r/28283/#comment105695>
Do we need these two configuration values in the configuration environment? These are used only for test purposes on QTestUtil. The user won't use these fields on hive-site.xml ever. Or not yet.
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java
<https://reviews.apache.org/r/28283/#comment105693>
I think we should leave the 'hadoop.encryption.is.not.supported' key name on unsupported hadoop versions. This was left only as a comment for developers. Nobody will use this configuration key anyways.
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java
<https://reviews.apache.org/r/28283/#comment105694>
Do we need these two configuration values in the configuration environment? These are used only for test purposes on QTestUtil. The user won't use these fields on hive-site.xml ever. Or not yet.
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/28283/#comment105697>
Let's import the necessary modules only. I think the IDE did this replacement.
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/28283/#comment105698>
Why was this block removed? I see the keyProvider variable is initialized inside getMiniDfs() method (testing). But what will happen with production code?
- Sergio Pena
On Nov. 28, 2014, 1:45 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2014, 1:45 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> ql/src/test/templates/TestEncrytedHDFSCliDriver.vm PRE-CREATION
> shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java ff7a82c
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63571
-----------------------------------------------------------
itests/qtest/pom.xml
<https://reviews.apache.org/r/28283/#comment105850>
Is this a typo? 'jalva'
The rest of the code looks very good!.
- Sergio Pena
On Dic. 2, 2014, 2:58 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dic. 2, 2014, 2:58 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63681
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On Dic. 3, 2014, 1:02 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dic. 3, 2014, 1:02 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
> On Dec. 5, 2014, 8:17 p.m., Brock Noland wrote:
> > Hi,
> >
> > This looks really good! I am very happy with the way this worked out! I have a few comments, some of which are nits and one or two "real" issues we need to adress. Thank you so much!
Thanks for your review. Update the patch according to your comments.
- cheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review64067
-----------------------------------------------------------
On Dec. 3, 2014, 1:02 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 1:02 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review64067
-----------------------------------------------------------
Hi,
This looks really good! I am very happy with the way this worked out! I have a few comments, some of which are nits and one or two "real" issues we need to adress. Thank you so much!
.gitignore
<https://reviews.apache.org/r/28283/#comment106479>
Are these files created by each test or should they be checked into source control?
If they are created each test we should use the "target" directory as data/files only contains files in source control.
If they should be in source control then I don't see them here and we'll not want to ignore them.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106480>
Since these are constants, the names should be in UPPER CASE, similar to QTEST_LEAVE_FILES below.
Also if they are not accessed outside this case we can make them private.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106481>
This is only used in this class and thus can be private.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106482>
This class has LOG which we should use as opposed to sysout.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106484>
I think we want the word "preserved" as opposed to "reserved".
If this is the case, please update the command for the funtion as well.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106485>
this is specified in initEncryptionRelatedConf as well. We can move it to a constant.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106486>
please use the constant you created above.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment106487>
please use the constant you created above.
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/28283/#comment106489>
In none testing enviroments getMiniDfs will never be called and this keyprovider will be null. Do we really need to delete this? If so, we need to find a way for this to work with production environments.
- Brock Noland
On Dec. 3, 2014, 1:02 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 1:02 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> .gitignore c5decaf
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review64638
-----------------------------------------------------------
Ship it!
Looks good Ferdinand.
- Sergio Pena
On Dic. 10, 2014, 2:21 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dic. 10, 2014, 2:21 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
> data/scripts/q_test_init_for_encryption.sql PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
> shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
(Updated Dec. 10, 2014, 2:21 a.m.)
Review request for hive.
Changes
-------
Brief summary for the update:
1. add back the keyProvider initial snippets in HdfsEncryptionShim
2. change the path of generated key to *target*
3. clean code
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs (updated)
-----
data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
data/scripts/q_test_init_for_encryption.sql PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 1:02 a.m.)
Review request for hive.
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs (updated)
-----
.gitignore c5decaf
data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
data/scripts/q_test_init_for_encryption.sql PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
(Updated Dec. 2, 2014, 2:58 a.m.)
Review request for hive.
Changes
-------
summary:
1. clean the code
2. remove unnecessary variables from HadoopShim
3. remove needless template
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs (updated)
-----
.gitignore c5decaf
data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
data/scripts/q_test_init_for_encryption.sql PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
(Updated Nov. 28, 2014, 1:45 a.m.)
Review request for hive.
Changes
-------
This patch consists of:
1. get the keyprovider via namefilesystem instead of initialize a new instance
2. add the encrption type for clusterType
3. set up three kinds of databases with encryption zones associated
4. do some code clean work
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs (updated)
-----
.gitignore c5decaf
data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION
data/scripts/q_test_init_for_encryption.sql PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION
ql/src/test/templates/TestEncrytedHDFSCliDriver.vm PRE-CREATION
shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java ff7a82c
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
> On Nov. 21, 2014, 8:25 p.m., Brock Noland wrote:
> > Hi Ferdindand,
> >
> > I think at the end of this JIRA we only want one q-file tests enabled. The other sub-tasks of HIVE-8065 are for enabling tests. I also think that as part of this test we'll need to startup a MiniDFS instance.
> >
> > Brock
Yes, the previous patch is not ready for review. And I have complete the patch and update this entry. This patch is tested locally by creating a simple database in the encrypted database. Please help me review it. Thanks!
> On Nov. 21, 2014, 8:25 p.m., Brock Noland wrote:
> > itests/qtest/pom.xml, line 546
> > <https://reviews.apache.org/r/28283/diff/2/?file=771833#file771833line546>
> >
> > Encrypted is misspelled
change to encrption?
- cheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review62626
-----------------------------------------------------------
On Nov. 21, 2014, 2:30 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 2:30 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> data/conf/encrypted/hive-site.xml PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review62626
-----------------------------------------------------------
Hi Ferdindand,
I think at the end of this JIRA we only want one q-file tests enabled. The other sub-tasks of HIVE-8065 are for enabling tests. I also think that as part of this test we'll need to startup a MiniDFS instance.
Brock
data/conf/encrypted/hive-site.xml
<https://reviews.apache.org/r/28283/#comment104702>
Do we actually need a new hive-site?
itests/qtest/pom.xml
<https://reviews.apache.org/r/28283/#comment104700>
Encrypted is misspelled
itests/src/test/resources/testconfiguration.properties
<https://reviews.apache.org/r/28283/#comment104701>
Actually I think we want to support just a few q-file tests. Specifically we should only be enabling a few q-file tests for each sub-task here:
https://issues.apache.org/jira/browse/HIVE-8065
- Brock Noland
On Nov. 21, 2014, 2:30 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 2:30 a.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The patch includes:
> 1. enable security properties for hive security cluster
>
>
> Diffs
> -----
>
> data/conf/encrypted/hive-site.xml PRE-CREATION
> itests/qtest/pom.xml 376f4a9
> itests/src/test/resources/testconfiguration.properties 3ae001d
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
> shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
>
> Diff: https://reviews.apache.org/r/28283/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> cheng xu
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/
-----------------------------------------------------------
(Updated Nov. 21, 2014, 2:30 a.m.)
Review request for hive.
Changes
-------
Remove the dependency of 2.6-snapshot from the QTestUtil.
Repository: hive-git
Description
-------
The patch includes:
1. enable security properties for hive security cluster
Diffs (updated)
-----
data/conf/encrypted/hive-site.xml PRE-CREATION
itests/qtest/pom.xml 376f4a9
itests/src/test/resources/testconfiguration.properties 3ae001d
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1
Diff: https://reviews.apache.org/r/28283/diff/
Testing
-------
Thanks,
cheng xu