----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60091/#review178634 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 269 (original), 280 (patched) <https://reviews.apache.org/r/60091/#comment252768> This is outside the scope of this issue, but I am really curios why this code uses lock? What can happen concurrently? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 295 (original), 306 (patched) <https://reviews.apache.org/r/60091/#comment252767> We can improve this a bit. Once we have the list authzPermissions.getAcls(authzObj), we can initialize retSet with the size of this list. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java Line 299 (original), 310 (patched) <https://reviews.apache.org/r/60091/#comment252765> What is the point of this operation? It seems that doing AddAll on an empty list is a noop - am I missing something here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java Lines 40 (patched) <https://reviews.apache.org/r/60091/#comment252772> Is there more then one renewer thread? If there are renewal threads for different purposes, it may be useful to pass consumer name as a constructor argument and add it to the name, e.g. kerberos-renewer-hms-follower, etc. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java Lines 48 (patched) <https://reviews.apache.org/r/60091/#comment252769> Do we need this? If we do, you can use 1-line version /** @deprecated */ sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java Line 107 (original), 125 (patched) <https://reviews.apache.org/r/60091/#comment252770> This doesn't work, use {} instead sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 81 (patched) <https://reviews.apache.org/r/60091/#comment252773> we shouldn't have more then one store-cleaner sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 122 (original), 128 (patched) <https://reviews.apache.org/r/60091/#comment252774> use {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 139 (original), 145 (patched) <https://reviews.apache.org/r/60091/#comment252775> use {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 155 (original), 161 (patched) <https://reviews.apache.org/r/60091/#comment252776> This probably should be a daemon sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 177 (original), 177 (patched) <https://reviews.apache.org/r/60091/#comment252777> Use {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 270 (original), 270 (patched) <https://reviews.apache.org/r/60091/#comment252778> Use {} sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 313 (patched) <https://reviews.apache.org/r/60091/#comment252780> This probably should be a daemon sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 314 (patched) <https://reviews.apache.org/r/60091/#comment252779> Why are we using ScheduledThreadPool rather then singleThreadedExecutor? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 384 (patched) <https://reviews.apache.org/r/60091/#comment252781> This probably should be a daemon, - we are trying to shut it down later sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 413 (original), 418 (patched) <https://reviews.apache.org/r/60091/#comment252782> Use explicit size for the new array - 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 > >
