impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Date Mon, 09 Oct 2017 19:55:14 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:55:14 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message