Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups ...................................................................... Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/frontend.h@156 PS11, Line 156: /// Returns (in the output parameter) the list of groups for the given user. > Comment Done 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@401 PS8, Line 401: const string& authorized_proxy_config_delimiter, : AuthorizedProxyMap* authorized_proxy_config_map > I'm not the expert here but I don't think we worry about the nullability as Done http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: () > 0) { > Yep, let's attach a timer to this and log it. Done http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc@1405 PS11, Line 1405: OG_QUERY << "Getting Hadoop groups took " << > Move out of the loop? Done http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc@1406 PS11, Line 1406: rettyPrinter::Print(end - start, > Can we move it before L1394? If this passes, we don't need to do a group lo Yeah good idea. Done. http://gerrit.cloudera.org:8080/#/c/10510/11/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10510/11/common/thrift/Frontend.thrift@834 PS11, Line 834: A flag to indicate wh > update Done http://gerrit.cloudera.org:8080/#/c/10510/11/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/11/fe/src/main/java/org/apache/impala/service/JniFrontend.java@627 PS11, Line 627: TGetHadoopGroupsRequest request = > Do we need to do this everytime? Oops. That was unintentional. Done. http://gerrit.cloudera.org:8080/#/c/10510/11/fe/src/main/java/org/apache/impala/service/JniFrontend.java@653 PS11, Line 653: * provider implementation. > Returns..... Done http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py@200 PS11, Line 200: @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args("--server_name=server1\ : --authorization_policy_file=%s\ : --authorized_proxy_user_config=hue=%s\ : --authorized_proxy_group_config=hue=%s\ : --abort_on_failed_audit_event=false\ : --audit_event_log_dir=%s" % (AUTH_POLICY_FILE, : getuser(), : ','.join(get_groups()), : AUDIT_LOG_DIR)) : def test_user_and_group_impersonation(self): : """End-to-end user and grou > I mean "custom delimiters are being parsed correctly." Will remove. Done. http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py@224 PS11, Line 224: er is 'hue' > We don't seem to be delegating it to a group user in __test_impersonation() Yes, for this particular test, the code path to getHadoopGroups will not be called. The goal is to make sure having the two authorized user and groups configs still work. I can remove this if you think this test doesn't make sense. -- 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: 13 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@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: Wed, 06 Jun 2018 22:56:56 +0000 Gerrit-HasComments: Yes
