[kudu-CR] Add "service name" as part of ConnectionId
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/11681 )
Change subject: Add "service_name" as part of ConnectionId
..
Patch Set 1:
(3 comments)
Thanks for doing this. Just a few comments.
http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h
File src/kudu/rpc/connection_id.h:
http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h@56
PS1, Line 56: // Copy state from another object to this one.
: void CopyFrom(const ConnectionId& other);
Looks like this isn't implemented. We can consider removing this.
http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc
File src/kudu/rpc/connection_id.cc:
http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@52
PS1, Line 52: string ConnectionId::ToString() const {
Need to incorporate service_name_ here too.
http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@69
PS1, Line 69: boost::hash_combine(seed, user_credentials_.HashCode());
We need to include the service_name_ as part of the HashCode too.
--
To view, visit http://gerrit.cloudera.org:8080/11681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:56:59 +
Gerrit-HasComments: Yes
[kudu-CR] Add compile time checks for kerberos, libvmem
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10870 ) Change subject: Add compile time checks for kerberos, libvmem .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/10870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0322e44784c026b6d6f1d3088bed03e95e94eb45 Gerrit-Change-Number: 10870 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 06 Jul 2018 16:53:53 + Gerrit-HasComments: No
[kudu-CR] Add krb5-server to SLES prerequisite libraries
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9727 ) Change subject: Add krb5-server to SLES prerequisite libraries .. Patch Set 1: Code-Review+2 Carry +2. I think we forgot to merge this. -- To view, visit http://gerrit.cloudera.org:8080/9727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12f78066f9dabe60d63dcea03a1ba22da942ac0f Gerrit-Change-Number: 9727 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 18 Apr 2018 21:31:17 + Gerrit-HasComments: No
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9934 ) Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. Patch Set 4: > Patch Set 4: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/12904/ : FAILURE Failure is a flaky test. -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 05 Apr 2018 23:34:44 + Gerrit-HasComments: No
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9934 ) Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. Patch Set 4: Code-Review+2 (4 comments) Carry +2. http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG@11 PS1, Line 11: cert.pem has 2 certificates in it: > I always thought this was the common case, where the trust store would cont Thanks, that makes sense. We weren't sure before this. http://gerrit.cloudera.org:8080/#/c/9934/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9934/3//COMMIT_MSG@28 PS3, Line 28: TODO: Add a test case that has multiple intermediate CAs. Right now we're testing > Maybe make a note in the JIRA about this TODO. I assume you want to land t Done http://gerrit.cloudera.org:8080/#/c/9934/3/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: http://gerrit.cloudera.org:8080/#/c/9934/3/src/kudu/security/test/test_certs.cc@509 PS3, Line 509: // The 'cert_file' here contains the serverCert and intermediateCA. > nit: refer to the function arguments instead. Done http://gerrit.cloudera.org:8080/#/c/9934/3/src/kudu/security/test/test_certs.cc@765 PS3, Line 765: // The 'cert_file' here contains the serverCert and intermediateCA. > same here Done -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 05 Apr 2018 23:02:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Hello Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9934 to look at the new patch set (#4). Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails Take 2 certificate files: cert.pem and truststore.pem cert.pem has 2 certificates in it: A cert for that node (with CN="hostname", and signed by CN=CertToolkitIntCA) And the intermediate CA cert (with CN=CertToolkitIntCA, and signed by CN=CertToolkitRootCA) truststore.pem has 1 certificate in it: A cert which is the root CA (with CN=CertToolkitRootCA, self-signed) This previously would not work with KRPC because in TlsContext::VerifyCertChainUnlocked(), we would only verify X509_verify_cert() with the top certificate in the server certificate chain. With this change, we pass the chain to X509_STORE_CTX_init() as well to make sure that the entire chain gets checked against the CA. A test is added that uses the specific certificate format mentioned above and added to rpc-test. TODO: Add a test case that has multiple intermediate CAs. Right now we're testing with only one intermediate CA. Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/security/tls_context.cc 4 files changed, 263 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/9934/4 -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9934 ) Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG@11 PS1, Line 11: cert.pem has 2 certificates in it: > I couldn't find any documentation on whether this is allowed or not. On the Mike Yoder commented on the JIRA saying that this should be supported, so I'm willing to take his word on that. http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG@28 PS1, Line 28: TODO: Add a test case that has multiple intermediate CAs. Right now we're testing : with only one in > nit: remove Done http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.h File src/kudu/security/test/test_certs.h: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.h@78 PS1, Line 78: // Same as the CreateTestSSLCertWithPlainKey() except that the 'cert_file' is > ca_cert_file is also different between the two. See my comment in the cc fi Done http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.cc@766 PS1, Line 766: // The kRootCaCert contains only the rootCA. > Maybe you could add some ascii-art to outline how these are signed and dist Done. I can work on adding a 4th cert, but that would require a regeneration of these certs, which can take sometime. I'll try to add it on as a following test case in a separate patch in the interest of time. -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 05 Apr 2018 22:15:19 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Hello Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9934 to look at the new patch set (#3). Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails Take 2 certificate files: cert.pem and truststore.pem cert.pem has 2 certificates in it: A cert for that node (with CN="hostname", and signed by CN=CertToolkitIntCA) And the intermediate CA cert (with CN=CertToolkitIntCA, and signed by CN=CertToolkitRootCA) truststore.pem has 1 certificate in it: A cert which is the root CA (with CN=CertToolkitRootCA, self-signed) This previously would not work with KRPC because in TlsContext::VerifyCertChainUnlocked(), we would only verify X509_verify_cert() with the top certificate in the server certificate chain. With this change, we pass the chain to X509_STORE_CTX_init() as well to make sure that the entire chain gets checked against the CA. A test is added that uses the specific certificate format mentioned above and added to rpc-test. TODO: Add a test case that has multiple intermediate CAs. Right now we're testing with only one intermediate CA. Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/security/tls_context.cc 4 files changed, 263 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/9934/3 -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9934 ) Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc@197 PS1, Line 197: cert.GetRawData()), "could not init X509_STORE_CTX"); > The quoted man page mentions that the stack is considered untrusted, so pre According to the above tutorial, self-signed certificates cannot be validated using these calls, i.e. a certificate will not get validated against itself: "It’s worth noting that self-signed certificates will always fail OpenSSL’s validation" -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 05 Apr 2018 22:03:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has removed Todd Lipcon from this change. ( http://gerrit.cloudera.org:8080/9934 ) Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. Removed reviewer Todd Lipcon. -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9934 )
Change subject: KUDU-2401: External TLS certificate with Intermediate CA in
server cert file fails
..
Patch Set 2:
(2 comments)
Thanks for the review Dan!
http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc@197
PS1, Line 197: cert.GetRawData()), "could not init
X509_STORE_CTX");
> I pretty much completely paged these APIs out so I started looking through
Good point. The man page was pretty ambiguous about that. The reason I did it
the first way was because of this para from the man page:
"X509_STORE_CTX_init() sets up ctx for a subsequent verification operation. It
must be called before each call to X509_verify_cert(), i.e. a ctx is only good
for one call to X509_verify_cert(); if you want to verify a second certificate
with the same ctx then you must call X509_STORE_CTX_cleanup() and then
X509_STORE_CTX_init() again before the second call to X509_verify_cert(). The
trusted certificate store is set to store, the end entity certificate to be
verified is set to x509 and a set of additional certificates (which will be
untrusted but may be used to build the chain) in chain. Any or all of the
store, x509 and chain parameters can be NULL."
https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html
But I'm guessing it probably means to do that one by one for certs that are not
part of a single chain.
I also looked at this tutorial to make sure that this is the right way to do it:
https://zakird.com/2013/10/13/certificate-parsing-with-openssl
I'll make that change.
http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc@201
PS1, Line 201: if (err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) {
> add braces around if block here and below.
I removed this code now.
--
To view, visit http://gerrit.cloudera.org:8080/9934
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4
Gerrit-Change-Number: 9934
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 05 Apr 2018 21:24:24 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Hello Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9934 to look at the new patch set (#2). Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails Take 2 certificate files: cert.pem and truststore.pem cert.pem has 2 certificates in it: A cert for that node (with CN="hostname", and signed by CN=CertToolkitIntCA) And the intermediate CA cert (with CN=CertToolkitIntCA, and signed by CN=CertToolkitRootCA) truststore.pem has 1 certificate in it: A cert which is the root CA (with CN=CertToolkitRootCA, self-signed) This previously would not work with KRPC because in TlsContext::VerifyCertChainUnlocked(), we would only verify X509_verify_cert() with the top certificate in the server certificate chain. With this change, we pass the chain to X509_STORE_CTX_init() as well to make sure that the entire chain gets checked against the CA. A test is added that uses the specific certificate format mentioned above and added to rpc-test. P.S: A majority of the change is testing related and the core code change is pretty small. Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/security/tls_context.cc 4 files changed, 262 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/9934/2 -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9934 Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails .. KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails Take 2 certificate files: cert.pem and truststore.pem cert.pem has 2 certificates in it: A cert for that node (with CN="hostname", and signed by CN=CertToolkitIntCA) And the intermediate CA cert (with CN=CertToolkitIntCA, and signed by CN=CertToolkitRootCA) truststore.pem has 1 certificate in it: A cert which is the root CA (with CN=CertToolkitRootCA, self-signed) This previously would not work with KRPC because in TlsContext::VerifyCertChainUnlocked(), we would only verify X509_verify_cert() with the top certificate in the server certificate chain. With this change, we iterate through the chain and try to verify each certificate with the CA. A test is added that uses the specific certificate format mentioned above and added to rpc-test. P.S: A majority of the change is testing related and the core code change is pretty small. Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/cert.cc M src/kudu/security/cert.h M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/security/tls_context.cc 6 files changed, 280 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/9934/1 -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] KUDU-2394. Fix flaky RpcStubTest.TestDontHandleTimedOutCalls
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9897 ) Change subject: KUDU-2394. Fix flaky RpcStubTest.TestDontHandleTimedOutCalls .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/9897/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9897/1//COMMIT_MSG@17 PS1, Line 17: Switching to 1000ms sleeps > The total test time is still just 1 second on average so I didn't think it nvm, I didn't see that you were sending only 'n_worker_threads_' RPCs. If you sent more RPCs than service threads, then the test would take longer, but that's not the case. -- To view, visit http://gerrit.cloudera.org:8080/9897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8cfebb5e9160fb11586abdbba6fc1ae06574a2c Gerrit-Change-Number: 9897 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Apr 2018 03:24:22 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2394. Fix flaky RpcStubTest.TestDontHandleTimedOutCalls
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9897 ) Change subject: KUDU-2394. Fix flaky RpcStubTest.TestDontHandleTimedOutCalls .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9897/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9897/1//COMMIT_MSG@17 PS1, Line 17: Switching to 1000ms sleeps Could this make it a slow test on an average case? And if yes, should we add it to the list of slow tests? -- To view, visit http://gerrit.cloudera.org:8080/9897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8cfebb5e9160fb11586abdbba6fc1ae06574a2c Gerrit-Change-Number: 9897 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 02 Apr 2018 23:54:58 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: fix multi-threaded test from running too long
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9856 ) Change subject: rpc-test: fix multi-threaded test from running too long .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9856 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I827a07d5b1d2463cb845ef6b04ec36036b2ddfbb Gerrit-Change-Number: 9856 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 29 Mar 2018 21:14:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9840 ) Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal() .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/9840/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9840/1//COMMIT_MSG@16 PS1, Line 16: confirmed on SLES11 that the new credential : is being inserted by checking the 'auth time' of the ticket : in ccache. nit: Could you also add that this was found on SLES11 because Impala has a slightly different ifdef which exercised that path on SLES11? https://github.com/apache/impala/blob/6f2ebadf8d119b1486f54b911ba3c7ecc1921d55/be/src/kudu/security/init.cc#L344-L352 -- To view, visit http://gerrit.cloudera.org:8080/9840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c Gerrit-Change-Number: 9840 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 28 Mar 2018 18:13:44 + Gerrit-HasComments: Yes
[kudu-CR] Add krb5-server to SLES prerequisite libraries
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9727 Change subject: Add krb5-server to SLES prerequisite libraries .. Add krb5-server to SLES prerequisite libraries I was running a test with Kudu on SLES and realized that the MiniKDC doesn't work due to 'kdb5_util' not being installed. Running "zypper install krb5-server" fixes this. Change-Id: I12f78066f9dabe60d63dcea03a1ba22da942ac0f --- M docs/installation.adoc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9727/1 -- To view, visit http://gerrit.cloudera.org:8080/9727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I12f78066f9dabe60d63dcea03a1ba22da942ac0f Gerrit-Change-Number: 9727 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9587 ) Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write() .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683 Gerrit-Change-Number: 9587 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 13 Mar 2018 04:34:13 + Gerrit-HasComments: No
[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9587 ) Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write() .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/9587/2/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9587/2/src/kudu/rpc/rpc-test.cc@1225 PS2, Line 1225: // This serves as a helper function for TestCancellationMultiThreads() to exercise : // cancellation when there are concurrent RPCs. nit: Better to not explain what a user of this function would do, since it's already explained in TestCancellationMultiThreads(). -- To view, visit http://gerrit.cloudera.org:8080/9587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683 Gerrit-Change-Number: 9587 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 13 Mar 2018 04:33:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9587 ) Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write() .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@186 PS1, Line 186: True if we have tried sending anything in 'payload_slices_' Actually isn't it more like "True if SendBuffer() is called at least once." ? The Writev() function can return without attempting to send anything in some cases. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@187 PS1, Line 187: // no bytes were sent successfully. This is needed as SSL_write() is stateful. nit: Add a reference to this JIRA, else it's hard to understand why we added it. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc@190 PS1, Line 190: started_ = true; Do we know of any cases where setting started_ = true this early can backfire? I looked at the code and it seems fine to me, but I just want to make sure. In some cases, we won't send anything at all: https://github.com/apache/kudu/blob/master/src/kudu/security/tls_socket.cc#L50-L55 Or we could hit errors before actually calling the SSL_write() or send() functions. So we should make sure that having started_ = true for these cases doesn't cause a problem elsewhere in the code. -- To view, visit http://gerrit.cloudera.org:8080/9587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683 Gerrit-Change-Number: 9587 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 12 Mar 2018 23:36:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9375 ) Change subject: KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc@118 PS5, Line 118: MutexLock l(lock_); > just because dump_stacks_now_reason_ is mutated from the logger thread too, Ah right, nvm then. -- To view, visit http://gerrit.cloudera.org:8080/9375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8 Gerrit-Change-Number: 9375 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 07 Mar 2018 21:19:35 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9375 )
Change subject: KUDU-2297 (part 5): dump stacks into diagnostics log on service
queue overflow
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.cc
File src/kudu/rpc/service_pool.cc:
http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.cc@136
PS5, Line 136: if (too_busy_hook_) {
: too_busy_hook_();
: }
You may get a less noisy stack if you call this before responding to the RPC.
Since responding to the RPC would start off some reactor thread tasks
asynchronously.
But the trade off is that you'll reply to this RPC a little later. Not a big
deal, but I just thought I'd bring it up.
http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc
File src/kudu/server/diagnostics_log.cc:
http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc@118
PS5, Line 118: MutexLock l(lock_);
Why bother locking this? It seems to me like when RunThread() gives up the lock
to do the actual logging, the 'dump_stacks_now_reason_' can be overwritten
anyway by another thread calling DumpStacksNow(). Unless I'm missing something.
--
To view, visit http://gerrit.cloudera.org:8080/9375
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8
Gerrit-Change-Number: 9375
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 07 Mar 2018 20:34:51 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
..
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> The '?' operator isn't working when I test it, unfortunately. Also, it appe
Interesting. That's fine by me then!
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:51:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> In that case, the {{#errors}} section would need to surround the whole Errors
UUID
Error
{{#errors}}
{{uuid}}
{{error}}
{{/errors}}
{{/errors}}
I'm not sure if Impala and Kudu use the same version of mustache, but it's
worth a try.
A final suggestion is to try this, but it may or may not be what you want. We
could display the static table headers unconditionally, and only fill in the
body through the mustache list:
Errors
UUID
Error
{{#errors}}
{{uuid}}
{{error}}
{{/errors}}
{{^errors}}
N/A
N/A
{{/errors}}
If we don't want to display even the table header when there are no errors,
then this doesn't apply.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:05:46 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2309: /masters can show the wrong list of masters
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9378 )
Change subject: KUDU-2309: /masters can show the wrong list of masters
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache
File www/masters.mustache:
http://gerrit.cloudera.org:8080/#/c/9378/4/www/masters.mustache@40
PS4, Line 40: {{^has_no_errors}}
> mustache doesn't have a way to condition based on the length of the 'errors
"An inverted section begins with a caret (hat) and ends with a slash. That is
{{^person}} begins a "person" inverted section while {{/person}} ends it.
While sections can be used to render text one or more times based on the value
of the key, inverted sections may render text once based on the inverse value
of the key. That is, they will be rendered if the key doesn't exist, is false,
or is an empty list"
https://mustache.github.io/mustache.5.html
I don't think the 'has_no_errors' field is necessary. The manual says if an
array is empty, it will execute the condition under {{^emptyarray}}.
So I imagine you could just have a regular section after:
{{#errors}}
...
...
{{/errors}}
{{^errors}}
No errors
{{/errors}}
Or something similar.
Alternatively, you could use Javascript to read the JSON and suppress HTML
fields accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/9378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I603ebc22e998bac9bd00edc939577ae339587f26
Gerrit-Change-Number: 9378
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 05 Mar 2018 21:23:41 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc@28 PS8, Line 28: #include > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:56:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Tidy Bot, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#9). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/9 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc@494 PS7, Line 494: ASSERT_OK(client_messenger->DumpRunningRpcs(dump_req, &dump_resp)); > Can you try looping this test with --stress-cpu-threads=$(nproc) to double I ran the test with --stress_cpu_threads=$(nrpoc) for about an hour (~800 iterations), and there were no failures. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:44:39 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#8). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/8 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: > I still don't think that helps, because the RPCs will still end up read off Ah right, that was a mistake. I changed it to keep the server's reactor thread busy by adding a sleep task for 2 seconds before queuing up RPCs. I tried using a latch, but the reactor thread restrictions hits a DCHECK, as we're not supposed to block on reactor threads. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:36:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#7). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/7 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/12071/ : FAILURE Failure is in an unrelated flaky test. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:45:42 + Gerrit-HasComments: No
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: controllers.back().get(), boost::bind(&CountDownLatch::CountDown, boost::ref(latch))); > Maybe I'm missing something here. If the main thread runs a bit slow before Yes you're right. I can't think of a sure shot way to make sure that we don't have this race. However, I've changed the RPC method to GenericCalculatorService::kSleepMethodName, with a 1 second sleep and 10 total RPCs. With 3 service threads (the default), that should give the main thread a little more than 3 seconds to dump the running RPCs information, which makes the flakiness very unlikely. I'm open to suggestions if this doesn't suffice. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:21:46 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#6). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/6 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#5). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 61 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/5 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#4). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 60 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/4 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9343 )
Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection
level
..
Patch Set 3:
(17 comments)
Since there's a lot of comments regarding the KB/s calculation and the rolling
window, I've removed that from this patch and will address all those comments
in a Part-2 patch.
This patch now only contains the outbound queue size metric. This is so that we
can get this patch in quickly and have some discussion before finalizing on
Part-2.
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@7
PS3, Line 7: metrics
> Maybe find a different word than "metrics" which already means a particular
Done
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@7
PS3, Line 7: KUDU-2031
> This one looks wrong.
Done
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@9
PS3, Line 9: rolling
: average of transfer speeds
> Can you point out where this would be helpful (and where it won't)? Naively
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@241
PS3, Line 241: double rough_kbps() const {
> why not to move the implementation into the connection.cc file?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@245
PS3, Line 245: for (int i = 0; i < rolling_window_size; ++i) {
: rough_kbps += rolling_kbps_[i];
: }
> syntactic sugar nit: I think it might be just
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
> Is this bit per second or byte per second? Please don't use abbreviations
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
> Can you talk a bit more about the use case for this? Do you think something
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82: rolling_kbps_.set_capacity(1000);
> move this to initialization list rolling_kbps_(1000) ?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82: 1000
> Does it make sense to control the size of the rolling window by a gflag? T
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> agreed grabbing the time at the start of each transfer and then the time wh
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> Yeah I think there's a danger that this is just timing how long it takes to
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> Not sure how accurate this will be fore inferring throughput given this is
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@471
PS3, Line 471: CHECK_OK
> why not just ASSERT_OK?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@472
PS3, Line 472: CHECK_OK
> ditto
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@502
PS3, Line 502: p.AsyncRequest(GenericCalculatorService::kAddMethodName,
add_req, &add_resp,
: controllers.back().get(),
boost::bind(&CountDownLatch::CountDown, boost::ref(latch)));
> we tend to use std::bind instead of boost::bind in newer tests, if possible
I'd have to change the signature of ResponseCallback in that case. Is that
acceptable?
https://github.com/apache/kudu/blob/0ce5ba594412de4365625485ea7b3c1ee21bf28d/src/kudu/rpc/response_callback.h#L26
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:
http://gerrit.cloudera.org:8080/#/c/9343
[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics
Hello Michael Ho, Lars Volker, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#3). Change subject: KUDU-2031: Add metrics per connection to the reactor metrics .. KUDU-2031: Add metrics per connection to the reactor metrics This patch returns the OutboundTransfer queue size and a rolling average of transfer speeds in Kbps on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that these metrics work as expected. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 7 files changed, 125 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/3 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics
Hello Michael Ho, Lars Volker, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#2). Change subject: KUDU-2031: Add metrics per connection to the reactor metrics .. KUDU-2031: Add metrics per connection to the reactor metrics This patch returns the OutboundTransfer queue size and a rolling average of transfer speeds in Kbps on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that these metrics work as expected. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 7 files changed, 125 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/2 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9343 Change subject: KUDU-2031: Add metrics per connection to the reactor metrics .. KUDU-2031: Add metrics per connection to the reactor metrics This patch returns the OutboundTransfer queue size and a rolling average of transfer speeds in Kbps on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that these metrics work as expected. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h 7 files changed, 121 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/1 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 ) Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client .. Patch Set 10: Code-Review+1 (2 comments) This LGTM http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123 PS5, Line 123: ead > It's probably not possible currently, but exception handling is non-local, Ok that makes sense. I'm fine with keeping it this way too. The reason I brought it up was because if some unexpected exception was thrown, we'd ideally want to fix it rather than transparently drop it. But I'm fine with it either way. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205 PS7, Line 205: > Yes, the 'ResetWriteBuf' method restores the prefix, and that's called at t Oops, missed that. Thanks for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/8692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345 Gerrit-Change-Number: 8692 Gerrit-PatchSet: 10 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 06 Feb 2018 04:34:17 + Gerrit-HasComments: Yes
[kudu-CR] jsonwriter: small optimization for PB fields
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 ) Change subject: jsonwriter: small optimization for PB fields .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9184 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31 Gerrit-Change-Number: 9184 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 03 Feb 2018 01:23:39 + Gerrit-HasComments: No
[kudu-CR] jsonwriter: small optimization for repeated PB fields
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9184 ) Change subject: jsonwriter: small optimization for repeated PB fields .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc File src/kudu/util/jsonwriter.cc: http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc@197 PS1, Line 197: ProtobufField(pb, field); Why not do the same here? I'm assuming pb.GetReflection() is non-inline-able as well? -- To view, visit http://gerrit.cloudera.org:8080/9184 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31 Gerrit-Change-Number: 9184 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 02 Feb 2018 23:34:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 ) Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client .. Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123 PS5, Line 123: ans > That doesn't catch all exceptions. I didn't realize we could hit non Thrift exceptions in Negotiate(), since SaslException is also a type of TException. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274 PS5, Line 274: > I'm not following, in this line needs_wrap_ is being retrieved from the SAS nvm, I misunderstood the fact that quth-int and auth-conf need to be negotiated. I thought we were forcing them on. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@141 PS7, Line 141: read_buf_.resize(kFrameHeaderSize); : transport_->readAll(read_buf_.data(), kFrameHeaderSize); Why not just read into a uint32_t here and avoid the resize(kFrameHeaderSize) ? Then in L154, you can do just the single resize and just put both the header and the payload at once into read_buf_. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205 PS7, Line 205: plaintext.remove_prefix(kFrameHeaderSize); Is this correct? If in L191, we figure that the write_buf_.size() > kSaslMaxBufLen, we first flush() a buffer of kSaslMaxBufLen, and we remove the prefix 'kFrameHeaderSize'. When the loop runs the second time, we're still removing the header prefix again. But the header will not be present in the buffer in second iteration of the loop. Unless I'm missing something. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@217 PS7, Line 217: size_t payload_len = write_buf_.size() - kFrameHeaderSize; Same here, in the second iteration of the loop, we'll be losing 'kFrameHeaderSize' bytes of information. -- To view, visit http://gerrit.cloudera.org:8080/8692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345 Gerrit-Change-Number: 8692 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 01 Feb 2018 22:08:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..
Patch Set 5:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h@82
PS5, Line 82: "127.0.0.1"
Are we sure that this won't clash with some other test services that may be
started on the same address ?
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h
File src/kudu/hms/sasl_client_transport.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h@130
PS4, Line 130: // Reads a frame from the underlying transport, storing the
payload into
: // read_slice_. If the connection is using SASL auth-conf or
auth-int
: // protection the data is automatically decoded.
: void ReadFrame();
> The biggest reason is that the framing isn't used during the SASL handshake
Ok makes sense. Thanks for the explanation.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@56
PS5, Line 56: kFrameHeaderWidth
nit: Same as Todd's comment above.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@107
PS5, Line 107: sasl_helper_.EnableGSSAPI();
Does this mean this won't work with SASL plain?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@120
PS5, Line 120: transport_->open();
DCHECK(!isOpen());
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ...
catch(TException& e) ?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@254
PS5, Line 254: !s.IsIncomplete() && !s.ok())
sasl_client_step() could return SASL_OK or SASL_CONTINUE, both of which are
valid statuses. This doesn't seem to take that into consideration.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274: needs_wrap_ = rpc::NeedsWrap(sasl_conn_.get());
Based on how you're initializing the transport, this looks like it should be
more like a DCHECK ? i.e. needs_wrap_ should always be true.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@284
PS5, Line 284: payload.size()
nit: Not that it's likely, but Slice objects can have lengths greater than can
fit in 32 bits. Something we want to add a DCHECK for ?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@292
PS5, Line 292: // Read the fixed-length message header.
DCHECK_EQ(payload.size(), 0);
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@343
PS5, Line 343: s = rpc::AllowProtection(sasl_conn_.get());
: if (!s.ok()) {
: throw SaslException(s);
: }
Integrity and confidentiality is set as a strict requirement here. Is that what
you want? Since sasl_encode() and sasl_decode() are known to be pretty slow.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@363
PS5, Line 363: "hive"
#define this with a comment stating that we'll only be talking to hive for now.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@378
PS5, Line 378: resize(0);
nit: clear()
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:11:21 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.6.x) rpc: allow setting --rpc tls min protocol on older RHEL versions
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9055 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.6.x Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 9055 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 18 Jan 2018 18:32:23 + Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 3: Code-Review+2 I tested this on platforms with OpenSSL 1.0.0 and 1.0.1e, by building on one and running on the other, and vice versa. And it does what is expected. We will do the same in Impala. -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 23:12:42 + Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 ) Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h File src/kudu/hms/sasl_client_transport.h: http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h@130 PS4, Line 130: // Reads a frame from the underlying transport, storing the payload into : // read_slice_. If the connection is using SASL auth-conf or auth-int : // protection the data is automatically decoded. : void ReadFrame(); One high level question before I delve into the details; why do you choose to implement your own framing vs. wrap around a TFramedTransport? -- To view, visit http://gerrit.cloudera.org:8080/8692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345 Gerrit-Change-Number: 8692 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 02 Jan 2018 19:43:02 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#10). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/CMakeLists.txt M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.cc A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/server/webserver_options.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 25 files changed, 580 insertions(+), 334 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/10 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 10 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 ) Change subject: KUDU-2228: Make Messenger options configurable .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8789/8/src/kudu/rpc/sasl_common.h File src/kudu/rpc/sasl_common.h: http://gerrit.cloudera.org:8080/#/c/8789/8/src/kudu/rpc/sasl_common.h@66 PS8, Line 66: Status SaslInit(bool kerberos_keytab_provided = false) WARN_UNUSED_RESULT; > warning: function 'kudu::rpc::SaslInit' has a definition with different par Done -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 13 Dec 2017 16:23:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#9). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/security/CMakeLists.txt M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.cc A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/server/webserver_options.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 23 files changed, 580 insertions(+), 334 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/9 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#8). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/security/CMakeLists.txt M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.cc A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/server/webserver_options.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 23 files changed, 580 insertions(+), 334 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/8 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 ) Change subject: KUDU-2228: Make Messenger options configurable .. Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc@272 PS7, Line 272: DCHECK_EQ(kerberos_enabled, is_kerberos_enabled); > I looked into this, turns out it's caused by SaslInit() calls from client_n Thanks. I made both the above changes and the failing tests seem to pass now. http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h File src/kudu/security/security_flags.h: http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@32 PS7, Line 32: class > nit: maybe, switch to struct since this does not have anything private/prot Done http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@34 PS7, Line 34: const char* > nit: const char* const ? Done http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/security/security_flags.h@35 PS7, Line 35: const char* > ditto Done -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 13 Dec 2017 15:40:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 ) Change subject: KUDU-2228: Make Messenger options configurable .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8789/7/src/kudu/rpc/sasl_common.cc@272 PS7, Line 272: DCHECK_EQ(kerberos_enabled, is_kerberos_enabled); Nearly all the external mini cluster kerberos tests hit this DCHECK. I'm not exactly sure how this code works, but it looks like the Messenger created here doesn't turn on Kerberos even if 'enable_kerberos' is true for the tests: https://github.com/apache/kudu/blob/master/src/kudu/mini-cluster/external_mini_cluster.cc#L154-L158 It wouldn't be turned on even before this patch because FLAGS_keytab_file for the test binary is an empty string. Does this look like an existing bug? Removing this DCHECK makes all the tests pass. -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 09 Dec 2017 06:49:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
..
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/messenger.cc@85
PS6, Line 85: enable_inbound_tls_(false) {
> This can be omitted since it's the default, and it's not a primitive type.
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/rpc/sasl_common.cc@271
PS6, Line 271: std::call_once(once, DoSaslInit, kerberos_enabled);
> Could you add a line after this call once:
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h
File src/kudu/security/security_flags.h:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@38
PS6, Line 38: } // namespace security
> I think this needs to be a const char*, and the value definition moved into
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/security/security_flags.h@48
PS6, Line 48:
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/6/src/kudu/server/server_base.cc@168
PS6, Line 168: DEFINE_string(rpc_tls_min_protocol,
> Can these flags use the constant?
Yup it can, I forgot to make the change earlier. Done.
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 23:06:01 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#7). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/CMakeLists.txt M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.cc A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/server/webserver_options.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 21 files changed, 581 insertions(+), 330 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/7 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc@78
PS4, Line 78: ert_(false) {
: security::InitializeOpenSSL();
: }
:
: TlsContext::TlsContext(std::string tls_ciphers, std::string
tls_min_protocol)
: : tls_ciphers_(std::move(tls_ciphers)),
: tls_min_protocol_(std::move(tls_min_protocol)),
: lock_(RWMutex::Priority::PREFER_READ
> Can think of two possible solutions:
Thanks for the suggestions. I went with the second option by creating the
defaults as static consts in security/security_flags.h
I also used these default members it in webserver_options.cc
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 21:54:13 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#6). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/server/webserver_options.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 19 files changed, 561 insertions(+), 330 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/6 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#5). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 18 files changed, 563 insertions(+), 321 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/5 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 ) Change subject: KUDU-2228: Make Messenger options configurable .. Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@168 PS3, Line 168: private: > Probably best to set these defaults in the constructor to keep things consi Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@176 PS3, Line 176: int64_t rpc_negotiation_timeout_ms_; > Is there any reason why FLAGS_rpc_tls_ciphers and FLAGS_rpc_tls_min_protoco Good catch, looks like I forgot to add them. I added them as options in the latest patch set. http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@74 PS3, Line 74: > nit: no need to comment this (it's obvious from context). Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@124 PS3, Line 124: > Seems more consistent to copy here too like other functions unless there is Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h@33 PS3, Line 33: > This doesn't appear to be used. I think I added it by mistake. It's not needed. http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@564 PS3, Line 564: void StartTestServer(Sockaddr *server_addr, > Wrap this like you did with 'CreateMessenger' Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@605 PS3, Line 605: template > ditto Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc@269 PS3, Line 269: > I think it may be cleaner to pass in the kerberos_enabled flag into the Onc Ah, thanks, I was trying to do that initially but couldn't because of GoogleOnceInit only allowing functions with no args. I changed it to std::call_once. http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/8789/4/src/kudu/security/tls_context.cc@78 PS4, Line 78: "ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:" : "ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:" : "ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:" : "ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:" : "ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:" :"AES256-GCM-SHA384:AES128-GCM-SHA256:" :"AES256-SHA256:AES128-SHA256:" :"AES256-SHA:AES128-SHA" I'm not thrilled about duplicating the defaults here and in MessengerBuilder, but if someone prefers a different way of doing this, I'm open to suggestions. http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc@52 PS3, Line 52: > this comment isn't necessary, we have a tool that checks headers. Done http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/util/flags.h@85 PS3, Line 85: Status ParseTriState(const char* flag_name, const std::string& flag_value, : TriStateFlag* tri_state); : : } // namespace kudu : #endif /* KUDU_UTIL_FLAGS_H */ : : : : : : : : : : > Is there any particular reason making this function template? Why not to d Good point. I'm not sure why it was a template. I had just moved this function from messenger.cc. In any case, it doesn't need to be a template, I made it non-templatized and moved it to flags.cc now. -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fr
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#4). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_tls_ciphers FLAGS_rpc_tls_min_protocol FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h M src/kudu/server/server_base.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h 18 files changed, 565 insertions(+), 320 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/4 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 ) Change subject: KUDU-2228: Make Messenger options configurable .. Patch Set 3: The last failure looks like a flaky test in: MultiThreadedHybridClockTabletTest/2.UpdateNoMergeCompaction -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 07 Dec 2017 15:53:30 + Gerrit-HasComments: No
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#3). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/flags.h 15 files changed, 443 insertions(+), 279 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/3 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable
..
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@434
PS1, Line 434: const std::string&
rpc_certificate_file = "",
> warning: the parameter 'rpc_certificate_file' is copied for each invocation
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@435
PS1, Line 435: const std::string&
rpc_private_key_file = "",
> warning: the parameter 'rpc_private_key_file' is copied for each invocation
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@436
PS1, Line 436: const std::string&
rpc_ca_certificate_file = "",
> warning: the parameter 'rpc_ca_certificate_file' is copied for each invocat
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@437
PS1, Line 437: const std::string&
rpc_private_key_password_cmd = "") {
> warning: the parameter 'rpc_private_key_password_cmd' is copied for each in
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@569
PS1, Line 569: DoStartTestServer(server_addr,
enable_ssl, rpc_certificate_file,
> warning: parameter 'rpc_certificate_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_ca_certificate_file' is passed by value and only co
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@570
PS1, Line 570: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_password_cmd' is passed by value and on
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@613
PS1, Line 613: CreateMessenger("TestServer",
n_server_reactor_threads_, enable_ssl, rpc_certificate_file,
> warning: parameter 'rpc_certificate_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_file' is passed by value and only copie
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_private_key_password_cmd' is passed by value and on
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/rpc/rpc-test-base.h@614
PS1, Line 614: rpc_private_key_file, rpc_ca_certificate_file,
rpc_private_key_password_cmd);
> warning: parameter 'rpc_ca_certificate_file' is passed by value and only co
Done
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/8789/1/src/kudu/server/server_base.cc@228
PS1, Line 228: } // namespace
> warning: anonymous namespace not terminated with a closing comment [google-
Done
--
To view, visit http://gerrit.cloudera.org:8080/8789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 14:34:46 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2228: Make Messenger options configurable
Hello Michael Ho, Tidy Bot, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8789 to look at the new patch set (#2). Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/flags.h 15 files changed, 443 insertions(+), 267 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/2 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot
[kudu-CR] [rpc] WIP: Introduce configurable options to Messenger
Sailesh Mukil has abandoned this change. ( http://gerrit.cloudera.org:8080/6520 ) Change subject: [rpc] WIP: Introduce configurable options to Messenger .. Abandoned Re-did the patch here: https://gerrit.cloudera.org/#/c/8789/ -- To view, visit http://gerrit.cloudera.org:8080/6520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I3685f137770d46f7c6537a37f76a0a6f71a00b11 Gerrit-Change-Number: 6520 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2228: Make Messenger options configurable
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8789 Change subject: KUDU-2228: Make Messenger options configurable .. KUDU-2228: Make Messenger options configurable Currently, the RPC layer accesses many gflags directly to take certain decisions, eg. whether to turn on encryption, authentication, etc. Since the RPC layer is to be used more like a library, these should be configurable options that are passed to the Messenger (which is the API endpoint for the application using the RPC layer), instead of the RPC layer itself directly accessing these flags. This patch converts the following flags to Messenger options and moves the flag definitions to server_base.cc which is the "application" in Kudu that uses the Messenger: FLAGS_rpc_default_keepalive_time_ms FLAGS_rpc_negotiation_timeout_ms FLAGS_rpc_authentication FLAGS_rpc_encryption FLAGS_rpc_certificate_file FLAGS_rpc_private_key_file FLAGS_rpc_ca_certificate_file FLAGS_rpc_private_key_password_cmd FLAGS_keytab_file Most of the remaining flags are test or benchmark related flags. There may be a few more flags that can be moved out and converted to options, but we can leave that as future work if we decide to move them. Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba --- M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/security/init.cc M src/kudu/security/init.h A src/kudu/security/security_flags.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/flags.h 15 files changed, 442 insertions(+), 267 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8789/1 -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8755 ) Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional .. Patch Set 5: (3 comments) Sorry for the late review, got caught up in some other work. LGTM for the most part, just a few comments. http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@432 PS5, Line 432: available nit: Should this be "enabled", since GSSAPI is available but just not used? http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@829 PS5, Line 829: RETURN_NOT_OK(check_gss_error(major, minor)); Are we leaking 'cred' if we return here? http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@834 PS5, Line 834: lifetime nit: remaining_life ? -- To view, visit http://gerrit.cloudera.org:8080/8755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276 Gerrit-Change-Number: 8755 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 07 Dec 2017 11:06:19 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8700 ) Change subject: [security] Make the kerberos principal configurable for Kudu servers .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/8700/5/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8700/5/src/kudu/security/init.cc@383 PS5, Line 383: *out_principal = in_principal; Forgot to initialize 'out_principal' here which led to the test failures. It's fixed now. -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 04 Dec 2017 22:49:32 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Hello Michael Ho, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8700 to look at the new patch set (#5). Change subject: [security] Make the kerberos principal configurable for Kudu servers .. [security] Make the kerberos principal configurable for Kudu servers The Kudu security library currently sources the kerberos principal directly from FLAGS_principal. Since this is a library, we'd rather move this to be an option that is passed through InitKerberosForServer(). Users of the security library may pass any principal that they want to. Testing: All current tests pass. Since this is a minor refactor, no new tests are needed. Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 --- M src/kudu/security/init.cc M src/kudu/security/init.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc 4 files changed, 30 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/8700/5 -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8700 ) Change subject: [security] Make the kerberos principal configurable for Kudu servers .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8700/3/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8700/3/src/kudu/security/init.cc@392 PS3, Line 392: out_principal); > Could you pass on out_principal instead of in_principal here? Ha, silly me, sorry. Done. -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 04 Dec 2017 22:19:04 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Hello Michael Ho, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8700 to look at the new patch set (#4). Change subject: [security] Make the kerberos principal configurable for Kudu servers .. [security] Make the kerberos principal configurable for Kudu servers The Kudu security library currently sources the kerberos principal directly from FLAGS_principal. Since this is a library, we'd rather move this to be an option that is passed through InitKerberosForServer(). Users of the security library may pass any principal that they want to. Testing: All current tests pass. Since this is a minor refactor, no new tests are needed. Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 --- M src/kudu/security/init.cc M src/kudu/security/init.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc 4 files changed, 29 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/8700/4 -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8700 ) Change subject: [security] Make the kerberos principal configurable for Kudu servers .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8700/3/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8700/3/src/kudu/security/init.cc@392 PS3, Line 392: const_cast(&in_principal) > Is it possible to assign in_principal to a local copy and then update that That's what I had done in the initial patchset, but I changed it based on Dan's comment. I'm fine with it either way. The one caveat this way when we use 'in_principal' directly, is that it amounts to modifying the FLAGS_principal itself. But that shouldn't cause any bugs. Let me know what you think is better. -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 04 Dec 2017 21:33:21 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8700 ) Change subject: [security] Make the kerberos principal configurable for Kudu servers .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/8700/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8700/2//COMMIT_MSG@12 PS2, Line 12: security > security Done http://gerrit.cloudera.org:8080/#/c/8700/2/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8700/2/src/kudu/security/init.cc@383 PS2, Line 383: const auto& kHostToken = "_HOST"; > I think you can skip this local copy altogether by using in_prinicipal and Done -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 04 Dec 2017 21:05:25 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Hello Michael Ho, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8700 to look at the new patch set (#3). Change subject: [security] Make the kerberos principal configurable for Kudu servers .. [security] Make the kerberos principal configurable for Kudu servers The Kudu security library currently sources the kerberos principal directly from FLAGS_principal. Since this is a library, we'd rather move this to be an option that is passed through InitKerberosForServer(). Users of the security library may pass any principal that they want to. Testing: All current tests pass. Since this is a minor refactor, no new tests are needed. Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 --- M src/kudu/security/init.cc M src/kudu/security/init.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc 4 files changed, 30 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/8700/3 -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8700 ) Change subject: [security] Make the kerberos principal configurable for Kudu servers .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8700/1/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8700/1/src/kudu/security/init.cc@382 PS1, Line 382: in_principal, > in_principal seems like a better name. Done -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 01 Dec 2017 19:57:38 + Gerrit-HasComments: Yes
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Hello Michael Ho, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8700 to look at the new patch set (#2). Change subject: [security] Make the kerberos principal configurable for Kudu servers .. [security] Make the kerberos principal configurable for Kudu servers The Kudu security library currently sources the kerberos principal directly from FLAGS_principal. Since this is a library, we'd rather move this to be an option that is passed through InitKerberosForServer(). Users of the sercurity library may pass any principal that they want to. Testing: All current tests pass. Since this is a minor refactor, no new tests are needed. Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 --- M src/kudu/security/init.cc M src/kudu/security/init.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc 4 files changed, 29 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/8700/2 -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho
[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8700 Change subject: [security] Make the kerberos principal configurable for Kudu servers .. [security] Make the kerberos principal configurable for Kudu servers The Kudu security library currently sources the kerberos principal directly from FLAGS_principal. Since this is a library, we'd rather move this to be an option that is passed through InitKerberosForServer(). Users of the sercurity library may pass any principal that they want to. Testing: All current tests pass. Since this is a minor refactor, no new tests are needed. Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 --- M src/kudu/security/init.cc M src/kudu/security/init.h M src/kudu/security/test/mini_kdc-test.cc M src/kudu/server/server_base.cc 4 files changed, 29 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/8700/1 -- To view, visit http://gerrit.cloudera.org:8080/8700 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695 Gerrit-Change-Number: 8700 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 ) Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506 PS2, Line 506: CreateTestSSLCertSignedByChain > Ah, yes, I see the server was given both certs. Sorry for misunderstanding Yes, both were given in the test. The intermediate and root CAs were passed through the out param 'ca_cert_file'. -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 20 Nov 2017 19:49:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 ) Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506 PS2, Line 506: CreateTestSSLCertSignedByChain > Which cert do we want do use, actually? Could we drop the old one? I basically added the intermediate cert (from 'kCaChainCert' below) to the 'kCert' variable. The reason is that if there is only one cert in the server/client certificate, then GetTopofChainX509() and GetEndOfChainX509() would both return the correct cert, since there's only one. But for all PEM files, if they're a chain of certs and not a CA, the sender certificate must come first, followed by a chain of certs that each trust the previous one in the chain. So, if we leave the function as GetEndOfChainX509() and add this intermediate cert to 'kCert', the test using these certs would fail. I think validation already happens now here at CheckPrivateKey(): https://github.com/apache/kudu/blob/master/src/kudu/security/cert.cc#L154-L159 If the cert we get back from GetTopOfChainX509() is incorrect, then this function would return an error. -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 20 Nov 2017 18:21:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 ) Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. Patch Set 2: > > Would it make more sense to just call it GetServerCertFromChain(), > > instead of GetTopofChainX509() ? > > As I understand, those wrapper functions could be used to work with > client-side certs, right? If so, then I don't think > GetServerCertFromChain() is the best choice here. Yes, you're right. The 'Cert' class is generic and not specific to server certs. I'll leave the name as it is then. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 20 Nov 2017 18:05:25 + Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 ) Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. Patch Set 2: Would it make more sense to just call it GetServerCertFromChain(), instead of GetTopofChainX509() ? -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 20 Nov 2017 17:51:09 + Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8595 to look at the new patch set (#2). Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. KUDU-2220: GetEndOfChainX509 does not return end-user cert KUDU-2091 introduced a function GetEndOfChainX509() which was supposed to return the "end-user" certificate. However, the end-user certificate is not at the end of the chain, but rather at the beginning of the chain as specificed by the RFC: https://tools.ietf.org/html/rfc5246#section-7.4.2 | This is a sequence (chain) of certificates. The sender's certificate MUST | come first in the list. Each following certificate MUST directly certify | the one preceding it. This patch fixes this by changing the GetEndOfChainX509() to GetTopOfChainX509(). An existing test is modified to test this patch. It does not pass without this change. Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/ca/cert_management.cc M src/kudu/security/cert.cc M src/kudu/security/cert.h M src/kudu/security/test/test_certs.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_handshake.cc 7 files changed, 65 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8595/2 -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8595 Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert .. KUDU-2220: GetEndOfChainX509 does not return end-user cert KUDU-2091 introduced a function GetEndOfChainX509() which was supposed to return the "end-user" certificate. However, the end-user certificate is not at the end of the chain, but rather at the beginning of the chain as specificed by the RFC: https://tools.ietf.org/html/rfc5246#section-7.4.2 | This is a sequence (chain) of certificates. The sender's certificate MUST | come first in the list. Each following certificate MUST directly certify | the one preceding it. This patch fixes this by changing the GetEndOfChainX509() to GetTopOfChainX509(). An existing test is modified to test this patch. It does not pass without this change. Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 --- M src/kudu/rpc/rpc-test.cc M src/kudu/security/ca/cert_management.cc M src/kudu/security/cert.cc M src/kudu/security/cert.h M src/kudu/security/test/test_certs.cc M src/kudu/security/tls_context.cc M src/kudu/security/tls_handshake.cc 7 files changed, 65 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8595/1 -- To view, visit http://gerrit.cloudera.org:8080/8595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7 Gerrit-Change-Number: 8595 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] WIP: tls socket: properly handle temporary socket errors in Writev
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8570 )
Change subject: WIP: tls_socket: properly handle temporary socket errors in
Writev
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc@99
PS1, Line 99: if (total_written > 0 &&
Socket::IsTemporarySocketError(write_status.posix_code())) {
: return Status::OK();
: }
> How do you know it will call it with the same length of 10?
I think the code as it is today will always call with the same length. The edge
case is if a partial write happens. But if Writev() does a partial write of an
iovec/slice, it returns back to the caller to adjust the offset within that
slice:
https://github.com/apache/kudu/blob/f9a6a1b2d15feb8e2c044f20248774105a3a62bf/src/kudu/security/tls_socket.cc#L87
So there wouldn't be a case where writev() calls SSL_write() with a different
length after a failure.
But I agree that's not in the contract and it's best not to depend on
implementation details that could change at any point, which may invalidate
this fix.
--
To view, visit http://gerrit.cloudera.org:8080/8570
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Gerrit-Change-Number: 8570
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Thu, 16 Nov 2017 19:49:33 +
Gerrit-HasComments: Yes
[kudu-CR] WIP: tls socket: properly handle temporary socket errors in Writev
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8570 )
Change subject: WIP: tls_socket: properly handle temporary socket errors in
Writev
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc@99
PS1, Line 99: if (total_written > 0 &&
Socket::IsTemporarySocketError(write_status.posix_code())) {
: return Status::OK();
: }
> Does this really help?
It would get the same parameters, wouldn't it?
If there is a vector with 2 frames of 10 bytes each, Writev() would call
Write() twice with a different buffer:
Write(buf, 10, ...); // succeeds
Write(buf+10, 10, ...); // fails
If the second call fails with 'SSL_ERROR_WANT_WRITE', we go back up to the
caller with Status::OK() and 10 bytes written. The caller would retry Writev()
with 'buf+10' as the new offset, so SSL_write() would see the same offset as it
last tried with.
--
To view, visit http://gerrit.cloudera.org:8080/8570
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472
Gerrit-Change-Number: 8570
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Thu, 16 Nov 2017 19:17:37 +
Gerrit-HasComments: Yes
[kudu-CR] WIP: tls socket: properly handle temporary socket errors in Writev
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8570 ) Change subject: WIP: tls_socket: properly handle temporary socket errors in Writev .. Patch Set 1: Code-Review+1 The fix makes sense to me. -- To view, visit http://gerrit.cloudera.org:8080/8570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472 Gerrit-Change-Number: 8570 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 16 Nov 2017 07:27:48 + Gerrit-HasComments: No
[kudu-CR] Allow configuration of values passed into kerberos env vars
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8247 ) Change subject: Allow configuration of values passed into kerberos env vars .. Patch Set 2: > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/10299/ : FAILURE Flaky test failure. -- To view, visit http://gerrit.cloudera.org:8080/8247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 Gerrit-Change-Number: 8247 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 13 Oct 2017 16:05:31 + Gerrit-HasComments: No
[kudu-CR] Allow configuration of values passed into kerberos env vars
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8247 )
Change subject: Allow configuration of values passed into kerberos env vars
..
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG@7
PS1, Line 7: Allow configuration of values passed into kerberos env vars
> typo
Done
http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG@15
PS1, Line 15: arguments to InitKerberosForServer().
> mind adding a sentence here saying _why_ you want to make them configurable
Yup, done.
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.h
File src/kudu/security/init.h:
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.h@41
PS1, Line 41: he kerberos replay cache by setting
> I think using boost::optional<> is nicer here
I reverted to just using the flag here now, so this is not necessary anymore.
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@470
PS1, Line 470: // per-connection server-generated nonce to protect against
replay attacks
> hrm, given that this is already configurable via a flag, why do we need it
That's true, I just thought it would be a little weird if some are configurable
via parameters and others and configured via flags.
But I guess this is something we should fix in the "make the Messenger
configurable" patch later.
I've removed the 'krb5_ktname' parameter and just used the flag now.
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@467
PS1, Line 467:
: if (disable_krb5_replay_cache) {
: // KUDU-1897: disable the Kerberos replay cache. The KRPC
protocol includes a
: // per-connection server-generated nonce to protect against
replay attacks
:
> maybe just scratch the defaults and instead change the call site in server_
I removed the krb5_ktname param, so we don't have this problem anymore. I left
the other 2 as defaults.
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@473
PS1, Line 473: setenv("KRB5RCACHETYPE", "none", 1);
> why do you need to configure this? We found that the replay cache was a hug
Yes, it's because the Thrift part would lack replay protection. I looked around
a bit and couldn't find a way to selectively disable it, since we're only able
to set it via an environment variable.
--
To view, visit http://gerrit.cloudera.org:8080/8247
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8247
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 13 Oct 2017 06:41:56 +
Gerrit-HasComments: Yes
[kudu-CR] Allow configuration of values passed into kerberos env vars
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8247 to look at the new patch set (#2). Change subject: Allow configuration of values passed into kerberos env vars .. Allow configuration of values passed into kerberos env vars We always used hardcoded constants for the following kerberos environment variables: KRB5CCNAME and KRB5RCACHETYPE. This patch allows for the configuration of these variables by taking arguments to InitKerberosForServer(). Callsites within Kudu have not been changed as all the parameters have default values. The motivation for this patch is that, Impala as a user of the KuduRPC and Kudu security libraries, needs to have a file based credential cache since the kinit happens on the C++ side and this cache needs to be read by the Java side too. Hence, we cannot have it in memory. Also, Impala still requires replay protection, since some Impala services use Thrift which lacks the nonce mechanism that KRPC uses for replay protection. Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 --- M src/kudu/security/init.cc M src/kudu/security/init.h 2 files changed, 19 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8247/2 -- To view, visit http://gerrit.cloudera.org:8080/8247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 Gerrit-Change-Number: 8247 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Allow configuration of values passed into kebreros env vars
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8247 ) Change subject: Allow configuration of values passed into kebreros env vars .. Patch Set 1: Looks like a flaky timeout failure in client-stress-test. It runs fine for me locally. -- To view, visit http://gerrit.cloudera.org:8080/8247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 Gerrit-Change-Number: 8247 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Tue, 10 Oct 2017 16:25:04 + Gerrit-HasComments: No
[kudu-CR] Allow configuration of values passed into kebreros env vars
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8247 Change subject: Allow configuration of values passed into kebreros env vars .. Allow configuration of values passed into kebreros env vars We always used hardcoded constants for the following kerberos environment variables: KRB5CCNAME, KRB5_KTNAME and KRB5RCACHETYPE. This patch allows for the configuration of these variables by taking arguments to InitKerberosForServer(). Callsites within Kudu have not been changed as all the parameters have default values. Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 --- M src/kudu/security/init.cc M src/kudu/security/init.h 2 files changed, 27 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8247/1 -- To view, visit http://gerrit.cloudera.org:8080/8247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 Gerrit-Change-Number: 8247 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
[kudu-CR] Allow configuration of values passed into kebreros env vars
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8247 )
Change subject: Allow configuration of values passed into kebreros env vars
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@467
PS1, Line 467: if (krb5_ktname.empty()) {
: setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
: } else {
: setenv("KRB5_KTNAME", krb5_ktname.c_str(), 1);
: }
This is a little ugly, but making the FLAGS_keytab_file available in the init.h
file for the sake of a default arg value, causes quite a few gflags problems.
--
To view, visit http://gerrit.cloudera.org:8080/8247
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8247
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Tue, 10 Oct 2017 06:26:59 +
Gerrit-HasComments: Yes
[kudu-CR] Allow the SASL protocol service name to be configurable
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/8218 )
Change subject: Allow the SASL protocol service name to be configurable
..
Patch Set 7:
(3 comments)
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/10235/ : FAILURE
Looks like a flaky failure in Test_KUDU_1735.
http://gerrit.cloudera.org:8080/#/c/8218/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
http://gerrit.cloudera.org:8080/#/c/8218/5/src/kudu/rpc/client_negotiation.cc@111
PS5, Line 111: std::string sasl_proto_name)
> warning: the const qualified parameter 'sasl_proto_name' is copied for each
Done
http://gerrit.cloudera.org:8080/#/c/8218/5/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
http://gerrit.cloudera.org:8080/#/c/8218/5/src/kudu/rpc/server_negotiation.cc@137
PS5, Line 137: std::string sasl_proto_name)
> warning: the const qualified parameter 'sasl_proto_name' is copied for each
Done
http://gerrit.cloudera.org:8080/#/c/8218/5/src/kudu/rpc/server_negotiation.cc@268
PS5, Line 268: Status ServerNegotiation::PreflightCheckGSSAPI(const
std::string& sasl_proto_name) {
> warning: the const qualified parameter 'sasl_proto_name' is copied for each
Done
--
To view, visit http://gerrit.cloudera.org:8080/8218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8218
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 06 Oct 2017 16:55:40 +
Gerrit-HasComments: Yes
[kudu-CR] Allow the SASL protocol service name to be configurable
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8218 to look at the new patch set (#7). Change subject: Allow the SASL protocol service name to be configurable .. Allow the SASL protocol service name to be configurable Previously the SASL service name was always set to a constant "kudu" which was tracked by kSaslProtoName in rpc/constants.h. However, for applications that use the KRPC library that would prefer to do their own SASL initialization, they would requre to set their own SASL service name to be passed into sasl_server_new()/sasl_client_new(). This patch allows for this configuration by adding a configurable parameter to the MessengerBuilder which is ultimately passed down to the negotiation layer. Change-Id: I9e30fe4461893b67527333259579e2304b19af1e --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/constants.cc M src/kudu/rpc/constants.h M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h 10 files changed, 59 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/8218/7 -- To view, visit http://gerrit.cloudera.org:8080/8218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8218 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Allow the SASL protocol service name to be configurable
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8218 to look at the new patch set (#6). Change subject: Allow the SASL protocol service name to be configurable .. Allow the SASL protocol service name to be configurable Previously the SASL service name was always set to a constant "kudu" which was tracked by kSaslProtoName in rpc/constants.h. However, for applications that use the KRPC library that would prefer to do their own SASL initialization, they would requre to set their own SASL service name to be passed into sasl_server_new()/sasl_client_new(). This patch allows for this configuration by adding a configurable parameter to the MessengerBuilder which is ultimately passed down to the negotiation layer. Change-Id: I9e30fe4461893b67527333259579e2304b19af1e --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/constants.cc M src/kudu/rpc/constants.h M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h 10 files changed, 59 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/8218/6 -- To view, visit http://gerrit.cloudera.org:8080/8218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8218 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Allow the SASL protocol service name to be configurable
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8218 to look at the new patch set (#5). Change subject: Allow the SASL protocol service name to be configurable .. Allow the SASL protocol service name to be configurable Previously the SASL service name was always set to a constant "kudu" which was tracked by kSaslProtoName in rpc/constants.h. However, for applications that use the KRPC library that would prefer to do their own SASL initialization, they would requre to set their own SASL service name to be passed into sasl_server_new()/sasl_client_new(). This patch allows for this configuration by adding a configurable parameter to the MessengerBuilder which is ultimately passed down to the negotiation layer. Change-Id: I9e30fe4461893b67527333259579e2304b19af1e --- M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/constants.cc M src/kudu/rpc/constants.h M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h 10 files changed, 59 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/8218/5 -- To view, visit http://gerrit.cloudera.org:8080/8218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8218 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Allow the SASL protocol service name to be configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8218 ) Change subject: Allow the SASL protocol service name to be configurable .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8218/4/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: http://gerrit.cloudera.org:8080/#/c/8218/4/src/kudu/rpc/client_negotiation.h@252 PS4, Line 252: std::string sasl_proto_name_; > const? Don't know why I didn't do that initially. Made the change. Sorry for the back and forth. -- To view, visit http://gerrit.cloudera.org:8080/8218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8218 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 05 Oct 2017 22:24:40 + Gerrit-HasComments: Yes
[kudu-CR] Allow the SASL protocol service name to be configurable
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8218 ) Change subject: Allow the SASL protocol service name to be configurable .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/8218/3/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/8218/3/src/kudu/rpc/negotiation.cc@182 PS3, Line 182: messenger->sasl_proto_name()); > sasl_proto_name() returns a 'const string&', and the ClientNegotiation ctor Ah right. Understood, misunderstood the fact that we're taking an owned string here. Thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/8218/3/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/8218/3/src/kudu/rpc/server_negotiation.cc@137 PS3, Line 137: std::string sasl_proto_name) > warning: pass by value and use std::move [modernize-pass-by-value] Done http://gerrit.cloudera.org:8080/#/c/8218/3/src/kudu/rpc/server_negotiation.cc@146 PS3, Line 146: sasl_proto_name_(std::move(sasl_proto_name)), > warning: parameter 'sasl_proto_name' is passed by value and only copied onc Done -- To view, visit http://gerrit.cloudera.org:8080/8218 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8218 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 05 Oct 2017 20:19:00 + Gerrit-HasComments: Yes
