Bharath Vissapragada 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: (13 comments) Just came across this CR. I have some comments. Will look at tests in the next round. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h@819 PS8, Line 819: /// Adds authorized proxy config into the given map. Maybe a little more verbose? The context is not clear by just reading this comment /// Utility to parse --foo and --bar and populate ..... http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h@820 PS8, Line 820: AddAuthorizedProxyConfig nit: How about we call it PopulateAuthorizedProxyConfig()? 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@320 PS8, Line 320: [](const string& config) { : return Substitute("Invalid proxy user configuration. No mapping value " : "specified for the proxy user. For more information review usage of the " : "--authorized_proxy_user_config flag: $0", config); : } I think using 'Status' as the return of AddAuthorizedProxyConfig() is an Impala way of doing this (caller checks for !status.ok()). I don't see why we should unnecessarily complicate this function signature with a lambda. thoughts? http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@401 PS8, Line 401: const string& authorized_proxy_config, : const string& authorized_proxy_config_delimiter Input params precede output vals [1]. Also we generally use output params as pointers. [1] https://google.github.io/styleguide/cppguide.html#Output_Parameters http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@425 PS8, Line 425: parsed_allowed_users_or_groups.begin(), parsed_allowed_users_or_groups.end()); nit: should we check for duplicate values instead of overwriting? ex: foo=bar;foo=car http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1386 PS8, Line 1386: TGetHadoopGroupsRequest req; Could you add a comment here that breiefly summarizes what we are doing http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: GetHadoopGroups I have seen issues around "GroupMapping" (especially with LDAP etc) taking longer than expected in the HDFS land. I'd assume we can run into similar issues, especially if we are calling this for every OpenSession(). Any thoughts on what we can do to help debug such issues without having to take thread dumps and then figuring it out. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1391 PS8, Line 1391: There was an nit: remove http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1397 PS8, Line 1397: AuthorizedProxyMap::const_iterator proxy_group = : authorized_proxy_group_config_.find(short_user); : if (proxy_group != authorized_proxy_group_config_.end()) { Move it outside of the loop? Also can we bypass the GetHadoopGroups() if this fails in the first place? http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1400 PS8, Line 1400: for (const string& group : proxy_group->second) { : if (group == "*" || group == do_as_group) return Status::OK(); : } Why are we looping here? Isn't this a set? Could we find() in O(1) ? 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@585 PS8, Line 585: GROUPS Is this thread safe? http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@641 PS8, Line 641: throw new InternalException(e.getMessage()); Can you LOG the full exception here. Helps debug issues with group mapping not working as expected. http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@672 PS8, Line 672: if (provider instanceof ShellBasedUnixGroupsMapping || : provider instanceof ShellBasedUnixGroupsNetgroupMapping) { Please add comments as to why these are problematic. -- 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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 06 Jun 2018 05:22:14 +0000 Gerrit-HasComments: Yes
