impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry
Date Tue, 21 Nov 2017 00:57:57 GMT
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 <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:57:57 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message