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-4129: Use KRPC's Kinit code to avoid expensive fork
Date Sat, 02 Sep 2017 00:39:22 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4129: Use KRPC's Kinit code to avoid expensive fork
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7938/2//COMMIT_MSG
Commit Message:

PS2, Line 9: subprocess
> child process
Done


PS2, Line 23:  util
> utility
Done


PS2, Line 33: Verified with thrift-server-test and also manually on a
            : live kerberized cluster.
> Did you verify that our current Jenkins AMI will be able to find the necess
I have a fix for our AMI and tested it with that fix, I'll update that separately as well.


http://gerrit.cloudera.org:8080/#/c/7938/2/CMakeLists.txt
File CMakeLists.txt:

PS2, Line 305: find_package(KerberosPrograms REQUIRED)
> Wouldn't build fail if it's not found ? Did you try packaging build ?
I've tested it on a machine without the binaries and the builds passed, but that may have
been on old bits. Let me run a packaging build on multiple platforms and see if it works fine.


http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/kudu/security/CMakeLists.txt
File be/src/kudu/security/CMakeLists.txt:

PS2, Line 89: set(SECURITY_TEST_SRCS_FOR_IMPALA
            :   test/mini_kdc.cc)
> nit: this can be one line
Done


http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

PS2, Line 74: Remove this flag in a compatibility-breaking release
> Is there a JIRA for that ?
Yes, I just filed IMPALA-5893.


Line 843:       KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(), "Could not init
kerberos");
> long line.
Done


http://gerrit.cloudera.org:8080/#/c/7938/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

PS2, Line 231: ASSERT_FALSE(non_ssl_client.Open().ok());
> Is it possible to match against the expected error status instead ?
Yes, I added a new macro that matches against an expected substring.


-- 
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message