----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59120/#review175051 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java Lines 73 (patched) <https://reviews.apache.org/r/59120/#comment248375> What is this change and how it relates to the issue? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 135 (patched) <https://reviews.apache.org/r/59120/#comment248376> The comment says "therefore return null here" but the code throws an exception instead. Do we really want to throw such exception? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 140 (patched) <https://reviews.apache.org/r/59120/#comment248377> in other places you call it metastore (one word), let's be consistent here and call it metastore as well. Also, I think it is sueful to log metastore URI in general, so why not make it an info log and say something like "Connecting to Hive metastore using URI {}" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 201 (patched) <https://reviews.apache.org/r/59120/#comment248379> We already have it - why do we need to read it again? sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java Line 19 (original), 19 (patched) <https://reviews.apache.org/r/59120/#comment248380> For some reason all these imports are showing up as unused - do you know why? sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java Lines 154 (patched) <https://reviews.apache.org/r/59120/#comment248381> Can you add a comment explaining what cache should be updated and why this is the value used? Also, please specify units for this delay. sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java Line 1216 (original), 1220 (patched) <https://reviews.apache.org/r/59120/#comment248382> This is rather unreliable - could there be some other way to wait for the condition which isn't time based? sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java Line 1342 (original), 1346 (patched) <https://reviews.apache.org/r/59120/#comment248383> Will it cause all HDFS tests to run longer? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java Lines 150 (patched) <https://reviews.apache.org/r/59120/#comment248384> Can you put your comments from RV to the code here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java Line 152 (original), 153 (patched) <https://reviews.apache.org/r/59120/#comment248385> Can you put your comment in the code? What are units for this wait? How did you come up with this formula? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java Lines 156 (patched) <https://reviews.apache.org/r/59120/#comment248386> What is this one and why it is needed? What are units? 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/#comment248387> The comment isn't very useful since it repeats the funciton name. It would be more useful to explain what is actually happening there. 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/#comment248388> Since you are changing the sequence of operations and it is important, please add a comment explaining the sequence of required operations. 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/#comment248389> 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()? 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/#comment248390> Why do we need this function? It is only called once. 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/#comment248393> Why we no longer need to start it as a privileged user? 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/#comment248392> WHy are we doing this? Why we are catching exception and then wrap it? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java Line 673 (original), 702 (patched) <https://reviews.apache.org/r/59120/#comment248391> Do we still need to run this as a priv user? 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/#comment248394> 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? - Alexander Kolbasov 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 > >
