Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-2660: Respect auth_to_local configs from hdfs configs ......................................................................
Patch Set 2: (4 comments) Thanks Alex for taking the first pass. Appreciate your reviews. I've posted some followup doubts on your comments, I'll upload a new patch once those are clarified. http://gerrit.cloudera.org:8080/#/c/2800/2/fe/src/main/java/com/cloudera/impala/authorization/User.java File fe/src/main/java/com/cloudera/impala/authorization/User.java: Line 32: static { > Does this mean that changing the auth_to_local config requires an Impala se Thats correct, hdfs requires a restart if we update this configuration. Line 59: } catch (IOException e) { > Can you add a comment here that describes at a high-level what is happening I'll update the comment here. Basically super.getShortName() throws NoMatchingRule() (extends IOEx) if no matching rule is found for a given principal. Do you think its better if wrap it as an ImpalaException and throw? Line 70: public String getShortNameForTesting(String rules) { > How about we handle the testing scenario as follows? I've considered that but the issue is "rules" are basically static, so if we use createTestUser(), subsequent new User() might see these newly set rules till we reset them. So we need to have another resetRules() to reset them back to normal. this method combines both these steps into one. What do you think? http://gerrit.cloudera.org:8080/#/c/2800/2/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: Line 1712: // running the query with authtest/[email protected] user which isconverted to user > typo: which is Done -- 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: 2 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: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
