----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60091/#review178592 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java Line 40 (original), 41 (patched) <https://reviews.apache.org/r/60091/#comment252685> We are usually avoiding wildcard imports, especially for classes with potentially big namespace lie java.util.concurrent sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java Lines 88 (patched) <https://reviews.apache.org/r/60091/#comment252687> The process name is sentry (it is running as part of Sentry server, so sentry part looks redundand. And it has nothing to do with hdfs - it builds HMS snapshot. so may be something like "hms-image-fetcher-%d' ? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java Lines 378 (patched) <https://reviews.apache.org/r/60091/#comment252692> Should we set it to be non-daemon as well? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Lines 89 (patched) <https://reviews.apache.org/r/60091/#comment252696> This isn't needed. LOG.debug handles this internally. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 86 (original), 90 (patched) <https://reviews.apache.org/r/60091/#comment252699> Sentry authorization is enforced on the following... sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Lines 94 (patched) <https://reviews.apache.org/r/60091/#comment252698> not needed. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 89 (original), 95 (patched) <https://reviews.apache.org/r/60091/#comment252697> It is better to avoid string join here. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 105 (original), 112 (patched) <https://reviews.apache.org/r/60091/#comment252701> Why do we need "" + ? Also, what is more cocher - character.toString or String.valueOf()? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 106 (original), 113 (patched) <https://reviews.apache.org/r/60091/#comment252703> we can use built-in formatting capability of preconditions. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 109 (original), 116 (patched) <https://reviews.apache.org/r/60091/#comment252704> Same comment about "" + ... sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 170 (original), 178 (patched) <https://reviews.apache.org/r/60091/#comment252705> As long as we can do it with {} specifiiers we don't need LOG.isDebugEnabled. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 223 (original), 234 (patched) <https://reviews.apache.org/r/60091/#comment252706> Do you think it should be daemon? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Lines 269 (patched) <https://reviews.apache.org/r/60091/#comment252707> What does this method do? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Lines 271 (patched) <https://reviews.apache.org/r/60091/#comment252708> What does it return? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Lines 272 (patched) <https://reviews.apache.org/r/60091/#comment252709> Do we need to duplicate deprecated status? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java Line 100 (original), 103 (patched) <https://reviews.apache.org/r/60091/#comment252694> This doesn't work - it should be "{}" instead of "%s" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java Line 216 (original), 220 (patched) <https://reviews.apache.org/r/60091/#comment252695> replace %s with {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java Lines 49 (patched) <https://reviews.apache.org/r/60091/#comment252682> I think this line has a trailing whitespace. Is there a value in duplicating @deprecated info? - Alexander Kolbasov On June 21, 2017, 2:53 p.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60091/ > ----------------------------------------------------------- > > (Updated June 21, 2017, 2:53 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1798 > https://issues.apache.org/jira/browse/SENTRY-1798 > > > Repository: sentry > > > Description > ------- > > SENTRY-1798: > Changed thread names and thread factory process to use the **guava** > _ThreadFactoryBuilder_ as a standard pattern for all thread Executors and > stand alone thread runs. > As well there are minor code cleanup per sonar in the files I was editting. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > cf9774c4fb5895df1fd3c26b9135ea5cad9344ef > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > 90ba72184ce55013c88905bf61314c908612b00a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > e0f8a9e959146a8828adcee5258a165d273eac1f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java > 8d78d1d4cadb1fa455a37bc8276793e9f231d691 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > ec938da1fb02bd0e58554336b671d14e7c6fc621 > > > Diff: https://reviews.apache.org/r/60091/diff/1/ > > > Testing > ------- > > Unit test runs and visual insepction of thread names when sentry is running. > > > Thanks, > > Brian Towles > >
