----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67560/#review204720 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 38 (patched) <https://reviews.apache.org/r/67560/#comment287376> Avoid the wilcard for the imports. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Line 89 (original), 97-99 (patched) <https://reviews.apache.org/r/67560/#comment287354> The 'true ==' in the conditions. It is enough to say: // is the Sentry word in the method name necessary? if (shouldIgnoreEvent()) { } Should pass the EventType.CREATE_TABLE parameter only without the toString() call? We can make the private method to accept that parameter as it is required for logging only. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 101-109 (patched) <https://reviews.apache.org/r/67560/#comment287358> This information is repeated in all the methods with slight changes based on the event type. Should we use a new class that builds this instead? Let's say a SentryHmsEvent class that has several constructors with different event types? Having a SentryHmsEvent is also better to avoid exposing the TSentryHmsEventNotification here. I should say we should expose Thrift classes only on the client class, but we can use this SentryHmsEvent as a wrapper of that class too. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 177-181 (original), 263-267 (patched) <https://reviews.apache.org/r/67560/#comment287361> There was a good explanation of what the method does, why was it removed? We should extend the old comment to mentio what it does now. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Line 182 (original), 268 (patched) <https://reviews.apache.org/r/67560/#comment287360> Is the notifyHmsEvent() better name? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 182-184 (original), 268 (patched) <https://reviews.apache.org/r/67560/#comment287362> What happened with this condition? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 339-344 (patched) <https://reviews.apache.org/r/67560/#comment287367> Would a map work better instead of these if conditions? Like: return mapToSentryOwnerType.get(principalType); We can later check in the shouldIgnoreEvent() if the type is null and just return and do not sync. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 345 (patched) <https://reviews.apache.org/r/67560/#comment287365> This should not be an error. This parameter would be useful for skipping events, and it might be logged as info only. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 360-366 (patched) <https://reviews.apache.org/r/67560/#comment287372> Should the annotations docs be filled with good description of them? API is not a subject. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 367 (patched) <https://reviews.apache.org/r/67560/#comment287368> Is the Sentry word needed? is it enough to say shouldIgnoreEvent()? It should be private too. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 416 (patched) <https://reviews.apache.org/r/67560/#comment287374> It should be private. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 417 (patched) <https://reviews.apache.org/r/67560/#comment287375> What if this method is used to pass an event that does not contain the DB_NOTIFICATION_EVENT_ID_KEY_NAME? The Long.parseLong will fail. I think we should check if that exists and return -1 if it doesn't. We should mention that in the comment as well. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java Lines 81-87 (patched) <https://reviews.apache.org/r/67560/#comment287395> You can use StringUtils.equals() from Apache commons here. It checks if the parameters are null and if not then it checks for equality on both. It is just one line of code. Also, is 'sergio' != 'Sergio' ? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 282 (patched) <https://reviews.apache.org/r/67560/#comment287396> This class is identical to the above class (onAlterSentryRoleGrantPrivilege) except that it accept a list of TSentryPrivilege objects. We should find a way to avoid this duplication. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Lines 409 (patched) <https://reviews.apache.org/r/67560/#comment287397> This class is very similar to the onAlterSentryRoleRevokePrivilege class, We should find a way to avoid this duplication. sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java Lines 279 (patched) <https://reviews.apache.org/r/67560/#comment287398> is notifyHmsNotifications() a good name? notifySentry is redundant as the client is called SentryClient.notifySentry and we know that every method calls Sentry, right? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1353 (patched) <https://reviews.apache.org/r/67560/#comment287400> Do we need another timer instead of grantTimer? This sync will do more things than just granting, and if the owner privilege flag is disabled, then it won't grant anything. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1365-1369 (patched) <https://reviews.apache.org/r/67560/#comment287401> Shouldn't the owner privilege be removed when an object is dropped? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1376 (patched) <https://reviews.apache.org/r/67560/#comment287402> There is an extra () in the first condition. If the owner is empty or null, then we're not going to remove any current owner privilege from the DB? I think we should treat empty and/or null as a posible parameter to mention to remove any owner privileges. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1387 (patched) <https://reviews.apache.org/r/67560/#comment287403> I don't think we should throw an exception if the HMS passed is not used by Sentry. We should log it instead as an info level that the requested event type is ignored. At the end, the request call is only to request an event. Sentry will do or will not do something with that event. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1411 (patched) <https://reviews.apache.org/r/67560/#comment287408> Add a comment about what this method does. Are you going to add the flag to check if the owner privilege is enabled? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1427-1428 (patched) <https://reviews.apache.org/r/67560/#comment287404> You can use the Collections.singleton() which returns a Set of a single object. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1437-1441 (patched) <https://reviews.apache.org/r/67560/#comment287407> Isn't it better to make one method call instead of if checks here? I think we can improve the method to allow empty or null privileges. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1438 (patched) <https://reviews.apache.org/r/67560/#comment287406> Is the grantor principal hive? We should not hardcode this value. Shouldn't we get the grantor from the caller which is HMS instead? HMS knows the user used to make this notification, doesn't it? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1453-1456 (patched) <https://reviews.apache.org/r/67560/#comment287405> Why throwing an exception and logging an error? We can avoid errors here ty making clear in the javadoc of the method that only ROLE and USER are accepted parameters and other will be ignored and logged. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1470 (patched) <https://reviews.apache.org/r/67560/#comment287409> Why do we need a timer here if this method is called from sentry_notify_hms_event() which has the same timer? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1486-1487 (patched) <https://reviews.apache.org/r/67560/#comment287410> You can use Collections.singleton(). sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1492 (patched) <https://reviews.apache.org/r/67560/#comment287411> Make a comment that multiple owners are not allowed and not expected but the Sentry database does not have constraints to prevent that, so it is possible (but unlikely) that multiple owners might be returned. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1493 (patched) <https://reviews.apache.org/r/67560/#comment287412> Space between for( sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1505-1507 (patched) <https://reviews.apache.org/r/67560/#comment287413> Shold we just log this as info and avoid throwing an exception? This is a private method and ideally it won't be called with any other type. Better yet, we should just ignore the type and not do anything. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1378-1382 (original), 1553-1557 (patched) <https://reviews.apache.org/r/67560/#comment287399> You can put this in a map to avoid if conditions, like: return mapSentryType.get(ownerType); You can check for the null value returned and do something in the caller. - Sergio Pena On June 13, 2018, 5:07 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67560/ > ----------------------------------------------------------- > > (Updated June 13, 2018, 5:07 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post > listener in HMS. This listener should be extended to get the owner > information of tables and databases. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java > ccb60ff2de16a00046e317d4b48fe37e23f7337e > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/metastore/TestSentrySyncHMSNotificationsPostEventListener.java > fc1c3d513f29bbc9dd2f8a3c6e1eeb4cd3e12e34 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > b5e01e4c21473523b494cc624318b73ec5732408 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > 6f38ed20dcba08ec1d5a4c63fadb0611381d9c98 > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > f0f08ea3d1f395ec973d735bfdb18b462f082afa > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 7f97ff7f054f797757aac94e243acdc5ee1127a2 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java > d8c82970b56b1599a07f0e26edab8ed3d59b9948 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f0e373a9aa5342d1b507e8b192cfdbc444242227 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > 6bfe872320d255acdb10f69e09c19623c04d8951 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > c056446262ddcf61db307eafbb785eac42973c80 > > > Diff: https://reviews.apache.org/r/67560/diff/1/ > > > Testing > ------- > > Added new tests and also made sure that existing tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
