Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11483 )
Change subject: IMPALA-7537: REVOKE GRANT OPTION regression ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@424 PS5, Line 424: rolePrivileges.add(catalog_.addRolePrivilege(roleName, privilege)); > This line may throw an exception because of either failure to get a client, Thanks for the explanation! Apart from Sentry and catalogd not being in sync (which was already a possibility), I see two possible issues: - the catalog will contain both grant and noGrant version of the privilege - the catalog will change, but the response will not be sent back to the client, so catalogd and the requesting impalad will be out of sync I am not sure if these are serious issues or not. Note that revokeRolePrivileges's hasGrantOption already has similar problems. http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@391 PS6, Line 391: // of mutliple column privileges. : TPrivilege tWithGrant = privileges.g > No. the buildPrivilegeName method requires the tWithGrant object which won The x.setPrivilege_name(...buildPrivilegeName(x)) pattern seems a bit weird to me, I had to read a few times to understand what is going on. Some ideas to make to code clearer: - PrincipalPrivilege could have a static function like RefreshPrivilegeName(TPrivilege) that does both the build + set name step - the whole logic could be extracted to a convenience function like TPrivilege CopyPrivilegeWithGrant(TPrivilege, bool) http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@472 PS7, Line 472: privWithGrant.setHas_grant_opt(true); Is it possible that privilege's grant_opt is already true? In that case the next lines try to remove the same privilege twice from the catalog. http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@476 PS7, Line 476: rolePrivNotGrant Is the naming correct here? We try to remove privWithGrant, while the result is rolePrivNotGrant. http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@479 PS7, Line 479: rolePrivileges.add(rolePriv == null ? rolePrivNotGrant : rolePriv); Is it possible in some extreme cases that both are non-null? For example if line 429 has thrown an exception. Probably it would be better to add both privileges in this case. -- To view, visit http://gerrit.cloudera.org:8080/11483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc Gerrit-Change-Number: 11483 Gerrit-PatchSet: 7 Gerrit-Owner: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 25 Sep 2018 10:02:40 +0000 Gerrit-HasComments: Yes
