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