----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65866/#review199044 -----------------------------------------------------------
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java Lines 354-359 (patched) <https://reviews.apache.org/r/65866/#comment279325> Why was it moved? Didn't make sense to have input and output privileges logged before checking for authorization? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java Lines 285-287 (patched) <https://reviews.apache.org/r/65866/#comment279328> is the LOG.isDebugEnabled() necessary? The LOG.debug() arguments are not doing any intensitve to have this validation, aren't they? And, what would 'this' print in the log? Should'nt this log being at the beginning of the method? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java Lines 304-306 (patched) <https://reviews.apache.org/r/65866/#comment279330> Same question about the LOG.isDebugEnabled(). sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java Line 343 (original) <https://reviews.apache.org/r/65866/#comment279331> Why was it removed? isn't it needed anymore? sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java Line 39 (original), 42 (patched) <https://reviews.apache.org/r/65866/#comment279333> Why are these unchecked exceptions part of the constructor declaration? I think that only checked exceptions are useful to have here. Unchecked exceptions are errors that are thrown that are not expected to happen, aren't they? Like a NullPointerException. sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java Lines 48 (patched) <https://reviews.apache.org/r/65866/#comment279332> Do we need this validation? privilegeStr is a simple string, so I don't think it causes harm if we leave the LOG.debug() call without checking if debug is enabled. sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java Line 23 (original), 23 (patched) <https://reviews.apache.org/r/65866/#comment279334> Same question about unchecked exceptions. sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java Line 26 (original), 26 (patched) <https://reviews.apache.org/r/65866/#comment279335> Same question about unchecked exceptions. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 112-115 (original), 111 (patched) <https://reviews.apache.org/r/65866/#comment279336> Why is this code removed? Isn't this patch only improving log messages? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 113 (patched) <https://reviews.apache.org/r/65866/#comment279338> Same question about isDebugEnabled. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 113 (patched) <https://reviews.apache.org/r/65866/#comment279341> Same question about isDebugEnabled. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 116 (patched) <https://reviews.apache.org/r/65866/#comment279337> Are we really getting privileges from cache? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 139-151 (patched) <https://reviews.apache.org/r/65866/#comment279339> How useful are these error messages for a user? What argument is illegal? Is there a way to handle these errors differently than just rethrowing the erros and logging the messages? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 139-151 (patched) <https://reviews.apache.org/r/65866/#comment279340> How useful are these error messages for a user? What argument is illegal? Is there a way to handle these errors differently than just rethrowing the erros and logging the messages? - Sergio Pena On March 1, 2018, 6:19 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65866/ > ----------------------------------------------------------- > > (Updated March 1, 2018, 6:19 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, kalyan kumar > kalvagadda, Liam Sargent, Na Li, Steve Moist, Sergio Pena, Vadim Spector, and > Xinran Tinney. > > > Repository: sentry > > > Description > ------- > > There are a bunch of improvements that should be made to > ResourceAuthorizationProvider. For example, exceptions thrown by > privilegeFactory.createPrivilege are not gracefully handled. Makes debugging > hard. > > We also need to add a lot more logging that needs to be added to related > classes > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 7565a34b5 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 09bd9b566 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > 447deaf58 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java > 4e944e5f2 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > ab5560994 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeFactory.java > 2f8296bfa > > sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPrivilegeFactory.java > d338f0edd > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > a9b98f319 > > > Diff: https://reviews.apache.org/r/65866/diff/2/ > > > Testing > ------- > > mvn -f sentry-binding/sentry-binding-hive/pom.xml test > mvn -f sentry-core/sentry-core-common/pom.xml test > mvn -f sentry-policy/sentry-policy-common/pom.xml test > mvn -f sentry-policy/sentry-policy-engine/pom.xml test > mvn -f sentry-provider/sentry-provider-common/pom.xml test > > > Thanks, > > Arjun Mishra > >
