Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: Status status = exec_env_->frontend()->GetHadoopGroups(req, &res); Does this code run even if the flag isn't set? If so, someone upgrading but using a "bad" UserGroupMapping provider will start getting the wrong message. http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@657 PS8, Line 657: protected static void checkGroupsMappingProvider(Configuration conf) We should be doing this at start-up rather than once per request. The issue isn't so much that it's expensive, but that you want to fail fast on this sort of configuration error. If we do it at boot time, we should also log the value of this configuration. http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@659 PS8, Line 659: GroupMappingServiceProvider provider = Isn't this just GROUPS? http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@665 PS8, Line 665: Map<String, String> recommendations = new HashMap<>(); : recommendations.put( : ShellBasedUnixGroupsMapping.class.getName(), : JniBasedUnixGroupsMappingWithFallback.class.getName()); : recommendations.put( : ShellBasedUnixGroupsNetgroupMapping.class.getName(), : JniBasedUnixGroupsNetgroupMappingWithFallback.class.getName()); This could go into the if below. But, really, just use a longer string. I think the clever map for recommendations takes away from the point. You can also just use an if/else for the two cases with the same number of lines. http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@685 PS8, Line 685: public String checkConfiguration() { This seems like a good place to check the user group mapping once and for all. Please look into what it does, of course. -- To view, visit http://gerrit.cloudera.org:8080/10510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Jun 2018 22:32:31 +0000 Gerrit-HasComments: Yes
