Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 )
Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc File be/src/kudu/security/test/mini_kdc.cc: http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@41 PS5, Line 41: // test_util.h has a dependancy on gmock which Impala doesn't depend on, so we rewrite : // parts of this code that use test_util members. : //#include "kudu/util/test_util.h" Doesn't this problem exist for all files which #include "test_util.h" ? I suppose we don't build kudu tests by default so this is an exception, right ? Otherwise, it'd be better in the long run to figure out how to resolve the dependency on gmock or we will keep doing the same change for every files which include this file. http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/test/mini_kdc.cc@66 PS5, Line 66: //options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc"); May as well delete this line instead of commenting it out. Add a comment instead. http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/auth-provider.h@143 PS5, Line 143: /// Runs "RunKinit" below if needs_kinit_ is true and FLAGS_use_krpc_kinit is false. Not your change but please clarify this is a long running thread which periodically forks Impala to run kinit. http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/authentication.cc@843 PS5, Line 843: KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(), : "Could not init kerberos"); May help to point out that a thread is created in this function which will periodically carry out kinit. What's the life cycle story for this thread ? http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@44 PS5, Line 44: #define ASSERT_KUDU_OK(status) \ : do { \ : const Status& status_ = (FromKuduStatus(status)); \ : ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \ : } while (0) > Should this live in gtest-util.h or kudu-util.h ? Also, based on the convention of KUDU_RETURN_IF_ERROR(), may be this one should be called KUDU_ASSERT_OK() ? -- To view, visit http://gerrit.cloudera.org:8080/7938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7 Gerrit-Change-Number: 7938 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Oct 2017 23:42:19 +0000 Gerrit-HasComments: Yes
