Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-2660: Respect auth_to_local configs from hdfs configs ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/2800/3/fe/src/main/java/com/cloudera/impala/authorization/AuthorizationChecker.java File fe/src/main/java/com/cloudera/impala/authorization/AuthorizationChecker.java: Line 117: throw new InternalException("Error calling getShortName() for user: " > I might be missing context from previous reviews - but why not make this an - I think this is not exactly an AuthzException since we didn't even try to authorize and failed even before doing it. Please let me know if you disagree. - Although, I agree its difficult to update all the callers to handle this InternalException. It took me a while to do it (especially tests). http://gerrit.cloudera.org:8080/#/c/2800/3/fe/src/main/java/com/cloudera/impala/authorization/User.java File fe/src/main/java/com/cloudera/impala/authorization/User.java: Line 36: getAuthenticationMethod > What authentication method is this referring to? Does Impala need to set th - These configs are set by CM (or whoever configures HDFS). Util classes used here are hdfs' utility classes to check if kerberos is configured in hadoop configs. https://github.com/cloudera/hadoop-common/blob/cdh5-2.3.0_5.1.0/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SecurityUtil.java#L607 - This code looks if kerberos has been configured in core-site.xml. My thinking was that we read these configs only if kerberos is configured at cluster level. Should this read Impala configs too? Line 39: defaultRule = "DEFAULT"; > Can you add a comment here explaining this logic? Done Line 62: public String getShortName() throws IOException { > Does this method need to exist if it just calls the parent class? (Is the m Previous patchsets had some logic here, now it just calls parent class. Not need anymore, removed. Line 80: // Do nothing, reset rules and return null. > comment doesn't really apply to this line of code. I think it's obvious wha Done http://gerrit.cloudera.org:8080/#/c/2800/3/fe/src/main/java/com/cloudera/impala/util/SentryPolicyService.java File fe/src/main/java/com/cloudera/impala/util/SentryPolicyService.java: Line 126: Error calling getShortName() > dropRole() shouldn't know what the internals of client.get().dropRole() are Agreed, changed here and elsewhere in this file. -- To view, visit http://gerrit.cloudera.org:8080/2800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76485b83c14ba26f6fce66e5f83e8014667829e0 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
