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