Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8588 )
Change subject: IMPALA-4927: Impala should handle invalid input from Sentry ...................................................................... Patch Set 4: (3 comments) What version of Sentry was causing this issue? Also, what was the exact exception being throw? http://gerrit.cloudera.org:8080/#/c/8588/4/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/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@141 PS4, Line 141: // need to be removed. It's fine to leave this as is, the 90 column limit is not exceeded. http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146 PS4, Line 146: sentryPolicyService_.listRolePrivileges(processUser_, role.getName())) { I think it would be better to call listRolePrivileges directly and catch exceptions from that instead of at the end of the function. If the role doesn't exist anymore, we definitely want to remove all its privileges from the catalog instead of going on to exception handling. http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@168 PS4, Line 168: } catch (Exception e) { Can you catch a more specific exception? For example, we probably wouldn't want to catch a NullPointerException here. -- To view, visit http://gerrit.cloudera.org:8080/8588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b Gerrit-Change-Number: 8588 Gerrit-PatchSet: 4 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2017 00:57:57 +0000 Gerrit-HasComments: Yes
