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