----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43137/#review118363 -----------------------------------------------------------
Fix it, then Ship it! sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java (line 315) <https://reviews.apache.org/r/43137/#comment179600> Nit: May be use a local variable for key as seems like we are looking it up multiple times. But, I guess performance wise it might not make much difference as entry.getKey might be just inlined? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java (line 63) <https://reviews.apache.org/r/43137/#comment179597> Nit: Whitespace sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 417) <https://reviews.apache.org/r/43137/#comment179601> From Sonar point of view this makes sense as it avoids the risk of caller modifying the array behind the back. But do you think it is needed given that HMSPaths is only called from trusted code? If so, can you add a comment mentioning the reason for copying the array. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java (line 117) <https://reviews.apache.org/r/43137/#comment179602> Nit: whitespace - Sravya Tirukkovalur On Feb. 3, 2016, 2:03 p.m., Colm O hEigeartaigh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43137/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 2:03 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Fix "Critical" issues identified by analysis.apache.org > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 5e2d8a1 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 7d56435 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > 617a8bc > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java > 1e83a6b > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 6066100 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > cb797af > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > 135ea20 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java > 4de130a > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > 107d3e1 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 6e14c29 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > e1c15fa > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java > 8e70ab7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > eac10a0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 521d945 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java > dd5880a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > c40edca > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java > fc7bc05 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java > 835e732 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFiles.java > d537e3b > > sentry-solr/solr-sentry-core/src/main/java/org/apache/solr/sentry/SentryIndexAuthorizationSingleton.java > c9d2414 > > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureCollectionsHandler.java > 7490ad0 > > Diff: https://reviews.apache.org/r/43137/diff/ > > > Testing > ------- > > > Thanks, > > Colm O hEigeartaigh > >
