> On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040271#file2040271line102> > > > > 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.
I have added SentryHmsEvent which abstracts the that logic. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040271#file2040271line279> > > > > 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. I have made changes to this logic in the new patch. This should be addressed. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040271#file2040271line284> > > > > What happened with this condition? I have made changes to this logic in the new patch. This should be addressed. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040271#file2040271line358> > > > > 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. If we had to perform this in mutiple places, I agree we should consctruct a static map with this data. That is not the case. I don't see a create a mapping. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > > Lines 282 (patched) > > <https://reviews.apache.org/r/67560/diff/1/?file=2040274#file2040274line282> > > > > 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. Assuming, you are talking about the overloaded implementation of onAlterSentryRoleRevokePrivilege API. I did not want to change the public API's. If you don't see any issue with that I can consolidate them. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > > Lines 409 (patched) > > <https://reviews.apache.org/r/67560/diff/1/?file=2040274#file2040274line409> > > > > This class is very similar to the onAlterSentryRoleRevokePrivilege > > class, We should find a way to avoid this duplication. done. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1353> > > > > 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. Added another metric. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1365> > > > > Shouldn't the owner privilege be removed when an object is dropped? This should be done as part of the notification handling. Currently, notificationProcessor class tries to remove privileges on DROP_DATABASE and DROP_TABLE events. That logic should be extended to handle owner privileges. We have another Jira to handle this. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1376> > > > > 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. I know we have a use case where we would want to remove the owner privilege but I prefer doing that differently. If we use empty/null as an input to remove owner privileges we can not differentiate a legitimate case or a case where did not receive owner information by error. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1411> > > > > Add a comment about what this method does. Are you going to add the > > flag to check if the owner privilege is enabled? Check is done in constructOwnerPrivilege. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1437> > > > > 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. There were existing public API's which i did not want to change. we can refactor it another patch. This patch is already big and i don't to want to increase the scope of it. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1438> > > > > 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? Typically grantorPrincipal is used to authorize if the user/group has permissions to grant an permission. I did not find this information in the hms events. Even If we could find a way to get that information from HMS, the user/group may not have permissions to grant. Let's take an example. user1 has create permission on the database db1 which allows him to create tables in database db1. user1 doesn't need to have grant permission. I have hard coded "Hive" for couple of reasons 1. In case of (create database/table) user is not granting the permissions explcitly. It is the service that granting it based on the user who is creating it. 2. In case of (alter) user performing alter just needs to have alter permission on server/database to be able to do it. 3. we need to actual grantor to perform authorization, in this case we are not performing any authozation. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1453> > > > > 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. I agree with this comment if SentryPolicyStoreProcessor is an public API where it doesn't have a control on what is passed. In this case in particular, the requests processed by SentryPolicyStoreProcessor are sent by sentry bindings. It's reasaobale to thow exceptions when it recived content that is not expected. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1470> > > > > Why do we need a timer here if this method is called from > > sentry_notify_hms_event() which has the same timer? You are right. It is not needed. it was stale code. I over looked it. > On June 13, 2018, 7:51 p.m., Sergio Pena wrote: > > 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/diff/1/?file=2040277#file2040277line1505> > > > > 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. That is my point, when it is not expected why not throw exception. If we just log and move-on we might some issues because they will not be noticed. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67560/#review204720 ----------------------------------------------------------- On June 15, 2018, 8:41 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67560/ > ----------------------------------------------------------- > > (Updated June 15, 2018, 8:41 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/SentryHmsEvent.java > PRE-CREATION > > 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/SentryMetrics.java > 5424bff67bc31d3f4ed2300531706e062f82b9ae > > 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/2/ > > > Testing > ------- > > Added new tests and also made sure that existing tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
