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 23:42:19 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:

(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 <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 23:42:19 +0000
Gerrit-HasComments: Yes

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