impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Date Tue, 10 Oct 2017 06:33:12 GMT
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 <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: Tue, 10 Oct 2017 06:33:12 +0000
Gerrit-HasComments: Yes

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