> On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Line 425 (original), 427 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line427> > > > > The comment isn't very useful since it repeats the funciton name. It > > would be more useful to explain what is actually happening there.
I will add more comment about what's going on > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Line 426 (original), 428 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line428> > > > > Since you are changing the sequence of operations and it is important, > > please add a comment explaining the sequence of required operations. I will add more detail in comment > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Line 459 (original), 465 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line468> > > > > Previously we were starting Hive as a privileged user. Why do we need > > to configure it as a privileged user? Can we move it all out of the doAs()? I don't know. Need to find it out later. Can we change it in another Jira, so we can get this in soon? > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Lines 546 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line549> > > > > Why do we need this function? It is only called once. It is for default input. Java does not support default value of input. http://stackoverflow.com/questions/997482/does-java-support-default-parameter-values > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Lines 692 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line695> > > > > Why we no longer need to start it as a privileged user? I did not realize the other calls are ran as privileged user. I think it works because those actions do not have access control check. > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Lines 695 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line698> > > > > WHy are we doing this? Why we are catching exception and then wrap it? This current code does not wrap it. The comment is not right. I will wrap the action in doAs(), then this comment and processing will be correct. > On May 16, 2017, 1:34 a.m., Alexander Kolbasov wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > > Lines 786 (patched) > > <https://reviews.apache.org/r/59120/diff/5/?file=1716366#file1716366line791> > > > > I don't understand this comment. Is there some async cleanup that is > > still going on? Why do you need to wait here? The cleanup should be done > > once we exit from this function. Or this isn't the case? I look at the code. A lot of processing are involded when removing HDFS, including RPC client. If we do't wait, it is easy to get exception "org.apache.hadoop.security.AccessControlException Permission denied: user=hive, access=EXECUTE, // inode="/tmp/external/p1":hdfs:hdfs:drwxrwx---" I suspect it is because The same file was deleted, but some resource associated to this file is not released yet. and creating it right away will be denied. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59120/#review175051 ----------------------------------------------------------- On May 12, 2017, 3:40 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59120/ > ----------------------------------------------------------- > > (Updated May 12, 2017, 3:40 a.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Sergio Pena. > > > Bugs: sentry-1757 > https://issues.apache.org/jira/browse/sentry-1757 > > > Repository: sentry > > > Description > ------- > > use hive configuration to check. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > 431c7fe > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 9880c40 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 1530eb2 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 7d128b7 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 32acc65 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 548dcf8 > > > Diff: https://reviews.apache.org/r/59120/diff/5/ > > > Testing > ------- > > integration test TestHDFSIntegrationEnd2End > > > Thanks, > > Na Li > >
