Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13346 )
Change subject: IMPALA-8476: Replace Sentry admin check workaround with proper Sentry API ...................................................................... Patch Set 11: (5 comments) Thanks for the fixes! This is getting close. I'm ready to give a +2 once all the comments are resolved. http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java: http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java@535 PS11, Line 535: public boolean isSentryAdmin(User requestingUser) throws InternalException, can we add javadoc? http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java@535 PS11, Line 535: throws InternalException, nit: I think it's more readable to put throws InternalException, SentryUserException in the next line. http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java@537 PS11, Line 537: SentryServiceClient client = new SentryServiceClient(); : try { : return client.get().isAdmin(requestingUser.getName()); : } finally { : client.close(); : } nit: using try-resource is shorter, i.e. try (SentryServiceClient client = new SentryServiceClient()) { return client.get().isAdmin(requestingUser.getName()); } http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/13346/11/fe/src/main/java/org/apache/impala/service/JniCatalog.java@304 PS11, Line 304: try { : boolean isSentryAdmin = ((SentryCatalogdAuthorizationManager) : catalogOpExecutor_.getAuthzManager()).isSentryAdmin(user); : response.setIs_admin(isSentryAdmin); : } catch (SentryUserException e) { : // When a user is not defined in Sentry, isAdmin() will throw : // SentryUserException, we will consider this requesting user : // as a non-Sentry administrator. : response.setIs_admin(false); : } Sorry for the back and forth, but putting this logic in JniCatalog doesn't feel quite right since JniCatalog is supposed to be a thin JNI wrapper. It makes more sense to move the try/catch logic in the SentryProxy instead. http://gerrit.cloudera.org:8080/#/c/13346/11/tests/authorization/test_sentry.py File tests/authorization/test_sentry.py: http://gerrit.cloudera.org:8080/#/c/13346/11/tests/authorization/test_sentry.py@51 PS11, Line 51: root nit: Calling it non_admin makes it easier to understand. -- To view, visit http://gerrit.cloudera.org:8080/13346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a27140d401494bc372ad0da96ada57bda94cd82 Gerrit-Change-Number: 13346 Gerrit-PatchSet: 11 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 26 Jun 2019 03:57:18 +0000 Gerrit-HasComments: Yes
