hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Brock Noland" <br...@cloudera.com>
Subject Re: Review Request 28283: HIVE-8900:Create encryption testing framework
Date Fri, 05 Dec 2014 20:17:09 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message