----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67560/#review204935 -----------------------------------------------------------
I think we should start thinking on splitting this patch into two, one for the client and another for the server. It would be faster to review the patches separately and address any changes from the feedback. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 62 (patched) <https://reviews.apache.org/r/67560/#comment287765> Why was this moved here? SentryHmsEvent should be only a holder, we should keep this logic on the SentryHmsSynsPostEventListener. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 65-69 (patched) <https://reviews.apache.org/r/67560/#comment287764> The skipEvent should be part of the SentryHMSSyncPostEventListener logic. SentryHmsEvent is only a holder of HMS event information, so let's keep it that way. The eventId will be -1 if there is not EVENT_ID assigned to the EventType, so we can use this -1 value later to check if an event is set or not. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 87 (patched) <https://reviews.apache.org/r/67560/#comment287766> Use StringUtils.equals() to compare nulls. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 106 (patched) <https://reviews.apache.org/r/67560/#comment287767> Should this be public? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 110 (patched) <https://reviews.apache.org/r/67560/#comment287768> Should this be public? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 116 (patched) <https://reviews.apache.org/r/67560/#comment287769> Could you add a comment that ownership in Hive 2.3.2 is only set for users? Better yet, put a TODO comment as a reminder when the Hive version is updated. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 145-153 (patched) <https://reviews.apache.org/r/67560/#comment287770> We have enough information to set the Thrift request. We should keep the SentryHmsEvent as a holder of information and build this TSentryHmsEventNotification to the Sentry client instead where all the Thrift request/response is used which IMO it is where it belongs. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 161-170 (patched) <https://reviews.apache.org/r/67560/#comment287773> We can use a map instead of if() sentendes. 1. It is more readable. 2. It gets the type in constant time. See this: private static final Map<PrincipalType, TSentryObjectOwnerType> mapOwnerType = ImmutableMap.of( PrincipalType.ROLE, TSentryObjectOwnerType.ROLE, PrincipalType.USER, TSentryObjectOwnerType.USER ); private TSentryObjectOwnerType getTSentryHmsObjectOwnerType(PrincipalType principalType) { return mapOwnerType.get(principalType); } If we don't want a type to be used, then we skip it in the SentryHMSPostEventListener. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 179-202 (patched) <https://reviews.apache.org/r/67560/#comment287772> We should put this logic back to the SentryHMSsyncPostEventListener. Let's keep this logic away of the holder object. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 218 (patched) <https://reviews.apache.org/r/67560/#comment287763> Gets (no Get`s). You should explain a little more from where it gets the event ID. Mention what constant is used and what value is expected. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHmsEvent.java Lines 223-229 (patched) <https://reviews.apache.org/r/67560/#comment287762> Optional: You can write this in one line if you use the getOrDefault() method: private long getEventId(ListenerEvent event) { return Long.parseLong( event.getParameters().getOrDefault(MetaStoreEventListenerConstants.DB_NOTIFICATION_EVENT_ID_KEY_NAME, "-1")); } sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Line 117 (original), 136 (patched) <https://reviews.apache.org/r/67560/#comment287750> getOwner() might return NULL. You can use the StringUtils.equals(str1, str2) to compare instead. Btw, are we going to treat 'Sergio' and 'sergio' as different users? Should we use equalsIgnoreCase() instead? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Line 120 (original), 139 (patched) <https://reviews.apache.org/r/67560/#comment287755> You can check for the owner type and owner name in this if as well. You don't have to split the if() conditions, just add another || and check for the type and owner name. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Line 122 (original), 141-142 (patched) <https://reviews.apache.org/r/67560/#comment287751> Why was it changed from equalsIgnoreCase() to equals()? Db1 and db1 are the same for Hive. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentrySyncHMSNotificationsPostEventListener.java Lines 148 (patched) <https://reviews.apache.org/r/67560/#comment287752> We should not throw an exception. Why aren't we allowing HMS to change the owner and a rename at the same time? HMS has an API that allows that. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Line 24 (original), 24 (patched) <https://reviews.apache.org/r/67560/#comment287776> Wildcard sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1362-1366 (patched) <https://reviews.apache.org/r/67560/#comment287778> Should we put a comment that the privileges, including the owner privilege, are dropped by the NotificationProcessor? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1368-1372 (patched) <https://reviews.apache.org/r/67560/#comment287779> Actually, HMS allows users to modify any metadata value of a table, so having both owner and table renames are expected. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1430-1431 (patched) <https://reviews.apache.org/r/67560/#comment287780> You can use a singleton here. Sets.singleton(ownerPrivilege). sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1441-1443 (patched) <https://reviews.apache.org/r/67560/#comment287781> I still think this 'hive' user should not be hard-coded. What if the HMS is running as a different user? Can we get the username from Sentry instead? We should have a way to get the user used to execute the Sentry server. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1472 (patched) <https://reviews.apache.org/r/67560/#comment287782> Can this method be merged with the grantOwnerPrivilege()? Granting an owner privilege will be assigned for a single role or user, so it can have the logic to revoke any current owner from the authorizable object. Most of the code is the same in the grantOwnerPrivilege. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 1377-1383 (original), 1554-1561 (patched) <https://reviews.apache.org/r/67560/#comment287783> You can use a map and keep the code readable. If we can do it now, we should then. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java Lines 70 (patched) <https://reviews.apache.org/r/67560/#comment287774> We should keep compatibility on this API. It is public exposed, isn't it? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 288-289 (patched) <https://reviews.apache.org/r/67560/#comment287784> Now that this flag is used here, should we add the flag that enables the owner privilege? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4473-4480 (original), 4613-4620 (patched) <https://reviews.apache.org/r/67560/#comment287786> Can this call the new method? { execute(Collections.singletonList(update), transactionBlock); } - Sergio Pena 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 > 45dce0e7caedc755c3b1b6515830974a21c11ee5 > > 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/3/ > > > Testing > ------- > > Added new tests and also made sure that existing tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
