----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40239/#review106275 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 224) <https://reviews.apache.org/r/40239/#comment165024> Coding style: if statements always use braces {} sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 236) <https://reviews.apache.org/r/40239/#comment165025> Braces sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 260) <https://reviews.apache.org/r/40239/#comment165026> Braces sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 262) <https://reviews.apache.org/r/40239/#comment165028> Braces sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 263) <https://reviews.apache.org/r/40239/#comment165027> Braces sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 272) <https://reviews.apache.org/r/40239/#comment165029> Braces sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 413) <https://reviews.apache.org/r/40239/#comment165030> Can we have a constant error message string where we just substitute the operation name? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 32) <https://reviews.apache.org/r/40239/#comment165032> It is usually a good practice to not import everything from a package. Can lead to unnecessary conflicts down the road. While you are here, can you also refactor the isManaged to isInPrefix? I think that wording makes more sense. - Sravya Tirukkovalur On Nov. 12, 2015, 5:42 p.m., Ryan Pridgeon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40239/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2015, 5:42 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-954: removeAclFeature/addAclFeature should only affect dirs under a > prefix which is managed. I did attempt to clean up SentryAuthorizatinProvider > a bit as well which may be a no no. Should be easy to revert these changes > without too much hassle > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java > 4d03ba3 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 208c93b > > Diff: https://reviews.apache.org/r/40239/diff/ > > > Testing > ------- > > Added testing to TestSentryAuthorizationProvider.java > > > Thanks, > > Ryan Pridgeon > >
