Sailesh Mukil 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: (17 comments) http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc File be/src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460 PS5, Line 460: //setenv("KRB5CCNAME", "MEMORY:kudu", 1); : //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1); : // We commented out the above since we don't want to overwrite our own Impala : // set environment variables. > Yes, I should have looked at the Kudu code base instead. Yes, I asked the Kudu guys and they said that they were okay with it. I posted a patch here: https://gerrit.cloudera.org/#/c/8247/ Once that's merged, I'll rebase this patch on top of that. 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 s Yes it does due to the dependenct on gmock. However, we don't include any of those files in our build. This would be the first. In the long run, I think we would fare better by including gmock as a dependency to pull in more of Kudu's test utils which I think are beneficial. 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 in Done 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 peri Done 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"); > Not sure how easy it is to do so but it'd be great to have a test which inj When a renewal fails, there's nothing we can do. The only thing that happens is that we add an error message to the logs and the cluster will eventually come to a halt. So, since there's no real error handling, I don't think a test for that would be necessary. What do you think? 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 Added a comment with the life cycle. 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) > Also, based on the convention of KUDU_RETURN_IF_ERROR(), may be this one sh Done 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 ? Moved to gtest-util.h http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@104 PS5, Line 104: int kdc_port = GetServerPort(); > Does this need to be global ? Yes, we need to get only the server port only once. If we make it a part of the class, we would get a new port for every test. http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@116 PS5, Line 116: > A class level comment explaining the purpose of this test class would be he Done http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@119 PS5, Line 119: GetParam() > Is this defined in the kudu library ? No, this is a function that's exposed by the Gtest library. It runs the test multiple times with the options in L189. http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@120 PS5, Line 120: FLAGS_use_krpc_kinit = (GetParam() == USE_KUDU_KERBEROS) ? true : false; > FLAGS_use_krpc_kinit = GetParam() == USE_KUDU_KERBEROS; Done http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@123 PS5, Line 123: filesystem::create_directories(unique_test_dir) > Should we check for return value of this ? Why don't we use the wrappers in I missed that function. I just changed it to use RemoveAndCreateDirectory() http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@157 PS5, Line 157: kudu::MiniKdc* kdc_ > Why not scope_ptr ? Done http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@174 PS5, Line 174: DCHECK(!kdc_); > DCHECK(kdc_ == nullptr); Done http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@176 PS5, Line 176: DCHECK(kdc_); > DCHECK(kdc_ != nullptr); Done http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@186 PS5, Line 186: ASSERT_KUDU_OK(kdc_->Stop()); > Are we leaking kdc_ too ? I think we were. I fixed it by making it a scoped_ptr. -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2017 06:33:12 +0000 Gerrit-HasComments: Yes
