Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11314 )
Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER ...................................................................... Patch Set 20: Code-Review+1 (3 comments) I added some suggestions to consider, lgtm otherwise. I do not have enough catalog experience to give +2, so I would like someone else look at it (at least at CatalogOpExecutor.java). http://gerrit.cloudera.org:8080/#/c/11314/20/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11314/20/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@999 PS20, Line 999: // If object ownership is enabled in Sentry, we need to update the owner privilege : // in the catalog so that any subsequent statements that rely on that privilege, or : // the absence, will function correctly without waiting for the next refresh. : // e.g. For create, the privileges should be available to immediately create a table. : // Additionally, if the metadata operation is successful, but sentry fails to add : // the privilege, it will be removed on the next refresh. ALTER DATABASE SET OWNER : // can be used to try adding the owner privilege again. : if (isObjectOwnershipEnabled()) { : Preconditions.checkNotNull(params.server_name); : TPrivilege filter = createDatabaseOwnerPrivilege(newDb.getName(), : params.server_name); : addPrivilegeToCatalog(newDb.getMetaStoreDb().getOwnerName(), : newDb.getMetaStoreDb().getOwnerType(), filter, resp); : } It would be nice to move similar blocks into separate functions. My idea would be to create a function like ChangeDbOwnership(old_owner, new_owner, db, server_name, resp), which could do add/remove/update depending on which ones from old_owner and new_owner are not null. (both set: change, new set: add, old set: remove). A similar function could be also created for tables. Even if merging turns out to be impractical, it may be worthwhile to create 6 functions for these operations, put them near each other, and add the comment only once. http://gerrit.cloudera.org:8080/#/c/11314/20/fe/src/test/resources/mysql-hive-site.xml.template File fe/src/test/resources/mysql-hive-site.xml.template: http://gerrit.cloudera.org:8080/#/c/11314/20/fe/src/test/resources/mysql-hive-site.xml.template@153 PS20, Line 153: <!-- These properties are for enabling the notifications between Hive and Sentry --> : <property> : <name>hive.metastore.transactional.event.listeners</name> : <value>org.apache.hive.hcatalog.listener.DbNotificationListener</value> : </property> : <property> : <name>hive.metastore.event.listeners</name> : <value>org.apache.sentry.binding.metastore.SentrySyncHMSNotificationsPostEventListener</value> : </property> : <property> : <name>hcatalog.message.factory.impl.json</name> : <value>org.apache.sentry.binding.metastore.messaging.json.SentryJSONMessageFactory</value> : </property> How is this change related to the patch? Maybe it could be mentioned in the commit message. http://gerrit.cloudera.org:8080/#/c/11314/20/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11314/20/tests/authorization/test_owner_privileges.py@250 PS20, Line 250: __root_query_fail As I see this functions are always used in pair with __verify_exceptions(). If __root_query_fail() would have an additional parameter like expected_error_msg, then the call sites could be simplified. -- To view, visit http://gerrit.cloudera.org:8080/11314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0 Gerrit-Change-Number: 11314 Gerrit-PatchSet: 20 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 05 Sep 2018 15:48:53 +0000 Gerrit-HasComments: Yes
