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: (10 comments) 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 ? 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 ? 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 helpful. 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 ? 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; 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 FileSystemUtil class instead ? 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 ? http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@174 PS5, Line 174: DCHECK(!kdc_); DCHECK(kdc_ == nullptr); http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/rpc/thrift-server-test.cc@176 PS5, Line 176: DCHECK(kdc_); DCHECK(kdc_ != nullptr); 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 ? -- 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: Mon, 09 Oct 2017 19:55:14 +0000 Gerrit-HasComments: Yes
