[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Alexey Serbin has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu .. Patch Set 2: (5 comments) Overall looks good to me, just some nits in the commit message. Maybe, Todd also want to take a look? http://gerrit.cloudera.org:8080/#/c/7662/2//COMMIT_MSG Commit Message: PS2, Line 15: 9) nit: please make the string <= 72 characters long PS2, Line 18: throught nit: typo PS2, Line 25: es, nit: please make the string <= 72 characters long http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() { : sk_X509_INFO_pop_free(info, X509_INFO_free); : }); > sk_X509_INFO_pop_free() takes 2 arguments but kFreeFunc is always passed on OK, I think it's good enough. PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will : // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr. : // We will take the responsibility of freeing it when we are done with the STACK_OF(X509). : stack_item->x509 = nullptr; > It would if we didn't set the pointer to nullptr, but since we set it to nu Ah, it seems that was my misunderstanding: I somehow missed the 'X509_INFO *stack_item = sk_X509_INFO_value(info, i)' assignment. Sorry. I think the way it's implemented now is fine. -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: (22 comments) http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); what about in the case of talking to an old server which doesn't support footer, but still mid-transmission? in that case don't we want to no-op this function call? here it seems like you are still aborting it, which would result in calling the callback while the sidecar is still referenced, and thus hitting the original bug again. PS2, Line 268: // Cancel or abort any outbound transfer for this call so references to the sidecars : // can be relinquished. per above, this is still best-effort in the case that you are talking to a remote which doesn't support this feature. PS2, Line 703:if (!transfer->TransferStarted()) { : if (transfer->is_for_outbound_call()) { : if (!StartCallTransfer(transfer)) { these can all be collapsed into a single boolean expression now, right? http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 199: takes typo: take PS2, Line 199: Callers should takes this into account. not quite sure what this sentence is indicating. Don't we assume that callers should always take everything documented into account? :) PS2, Line 201: negotiation_complete_ && is negotiation_complete_ necessary? before negotiation is complete, isn't remote_features_ guaranteed to be an empty set, in which case it will already be false? PS2, Line 284: sending it for : // the first time I think better to say "beginning to send it" or "beginning transmission" or somesuch Line 288: // If the checks pass, the outbound call will be set to 'SENDING' state. if the checks don't pass, is this responsible for 'finishing' the call? eg if it doesn't support the feature flags, will this take care of calling the callback? PS2, Line 289: Append nit: "Appends" PS2, Line 303: if the transfer is for an outbound call. not quite clear here which part of the sentence the "if ..." applies to. If I understand correctly, maybe this should say: // Remove the front transfer from the outbound transfers queue. // If the transfer is associated with an outbound call, clears the transfer reference from the associated CallAwaitingResponse. or something like that, to be more clear that it _always_ removes the front, and then the reference removal is in addition to that, in certain circumstances. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: Line 30: #include "kudu/rpc/connection.h" fix sort order PS2, Line 260: shutted nit: "shut" Line 266: // fall-through if remote supports parsing footer. we have a special macro here 'FALLTHROUGH_INTENDED' which turns into something magic in clang (see gutil/macros.h) PS2, Line 352: delete [] header_buf_.release(); : delete [] footer_buf_.release(); looking at the implementation of faststring::release() I think these calls are actually not a great idea, since, if the data is less than 32 bytes, it will be stored inline in the faststring object, and then these 'release' calls actually cause an allocation and copy into a heap buffer. For the header, I think it's typically >32 bytes so this is beneficial, but for the footer it's almost certainly not. I think it's better to change both of these to call clear() followed by shrink_to_fit(), which will free the memory only if it's a separate allocation. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS2, Line 159: send_footer is this out of date? I don't see this param Line 171: // to indicate that the call has been aborted. worth indicating whether this call requires that AppendFooter() has previously been called. What happens if no AppendFooter() was called? would it CHECK fail? (probably should?) PS2, Line 210: IsBeingSent hrm, I actually am going to disagree with my previous comment and still think this isn't a great name, since it includes the "scheduled but hasn't started sending". Maybe "IsQueuedForTransfer" or 'IsOnTransferQueue' or something? http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: PS2, Line 274: Note that the footer is designed to be modified : // after the initial serialization and it will be re-serialized after modification. : // To avoid unexpected change in the total message size, keep to using fixed sized : // fields only in the footer. move this comment to be below the 'aborted' field, since it documents any new fie
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Sailesh Mukil has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: Line 37: #include "kudu/util/status.h" > warning: #includes are not sorted properly [llvm-include-order] Done PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() { : sk_X509_INFO_pop_free(info, X509_INFO_free); : }); > nit: if you introduced corresponding trait functions for STACK_OK(X509_INFO sk_X509_INFO_pop_free() takes 2 arguments but kFreeFunc is always passed one argument, so this will not work unless we create a wrapper function for sk_X509_INFO_pop_free(), which I think is going beyond what we need since we use it only in a single place. Let me know what you think. PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will : // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr. : // We will take the responsibility of freeing it when we are done with the STACK_OF(X509). : stack_item->x509 = nullptr; > Would the scoped cleanup about try to free the data just put into the stack It would if we didn't set the pointer to nullptr, but since we set it to nullptr, it will ignore it. Another way of doing this would be to use sk_X509_dup() above instead of sk_X509_push() to copy the X509 items over to the final stack, and not do the following line: stack_item->x509 = nullptr; But I thought this would be better for performance in the case of large chains. PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) { : return nullptr; : } : return sk; > I think it should be Ah yes, this seems the right way to do it. Done. -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7662 to look at the new patch set (#3). Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu .. KUDU-2091: Certificates with intermediate CA's do not work with Kudu Kudu previously did not recognize chain certificates. This patch enables support for chain certificates by changing the Cert class' underlying data type to STACK_OF(X509) instead of just X509. STACK_OF(X509) allows multiple certificates to be held by the same pointer. When we are presented with a file or a string that contains multiple X509 certificates, they will be stored inside this STACK_OF(X509) object. When we call AddTrustedCertificate(Cert&), we iterate throught the STACK_OF(X509) contained in the Cert and add each one individually to the X509_STORE for later verification. Currently, IPKI does not make use of this ability and still works with single certificates. DCHECKS are added to make sure that multiple X509 certificates are not accidentally added to a Cert object. Although this patch provides a general framework to use chain certificates, if we want to use IPKI with chain certificates, additional functionality will need to be added with clearer APIs. External PKI makes use of this ability to add a chain CA if necessary. Testing: A new test is added to rpc-test that uses a chain CA. This test does not work without this patch. Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f --- 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/openssl_util.cc M src/kudu/security/openssl_util.h M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_handshake.cc 10 files changed, 430 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/3 -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build-support] added IWYU filter script
Alexey Serbin has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 8: > Thanks. Maybe Dan wants to take a look, since he's reviewing your > other IWYU patches? Yes, I would appreciate if Dan could take a look at this as well. -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Alexey Serbin has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: PS1, Line 178: ASSERT_OK(DoTestSyncCall(p, Ge > I think it should be fine, but I copied the above TestCall() test. I guess Just was curious whether it was necessary to have it here, but that was not crucial and you removed it already, that's fine. http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 214: DCHECK(cert); > Unfortunately, X509_chain_up_ref() is only available from OpenSSL 1.1.0. Sh I think you can leave it as is -- I was just thinking that the code might be simplified. If not and it requires additional ifdefs, then it's better to keep it as is. http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() { : sk_X509_INFO_pop_free(info, X509_INFO_free); : }); nit: if you introduced corresponding trait functions for STACK_OK(X509_INFO), that could be handled via ssl_make_unique(). PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will : // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr. : // We will take the responsibility of freeing it when we are done with the STACK_OF(X509). : stack_item->x509 = nullptr; Would the scoped cleanup about try to free the data just put into the stack upon exiting from the scope? PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) { : return nullptr; : } : return sk; I think it should be if (sk_X509_push(sk, x.get()) == 0) { return nullptr; } x.release(); return sk; The idea here is to avoid memory leak if sk_X509_push() fails. I looked into the sk_X509_push() and it can fail if it cannot grow the stack to accommodate the new element or the passed stack pointer is nullptr. If so happens, the caller retains the ownership of the passed data/new element. -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Adar Dembo has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 8: Code-Review+2 Thanks. Maybe Dan wants to take a look, since he's reviewing your other IWYU patches? -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [build-support] added IWYU filter script
Adar Dembo has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/7604/7/build-support/release/rat_exclude_files.txt File build-support/release/rat_exclude_files.txt: Line 14: build-support/iwyu/mappings/boost-mappings-LICENSE.TXT This file no longer exists. -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7604 to look at the new patch set (#8). Change subject: [build-support] added IWYU filter script .. [build-support] added IWYU filter script Added a script to filter the output from the include-what-you-use tool. The idea is that the IWYU tool v0.8 is of alpha quality and produces many incorrect recommendations. Most of those can be addressed by using various 'IWYU pragma' directives, but not in the case of auto-generated files. Adding those directives is a very tedious job, and only some of those places are addressed. Moreover, the incorrect IWYU recommendations seem to be a little bit different version from version. Also, added IWYU mappings for the boost library (imported from the IWYU source tree) and the glog, gflags, gtest libraries. Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa --- A build-support/iwyu/iwyu-filter.awk A build-support/iwyu/mappings/boost-all-private.imp A build-support/iwyu/mappings/boost-all.imp A build-support/iwyu/mappings/gflags.imp A build-support/iwyu/mappings/glog.imp A build-support/iwyu/mappings/gtest.imp M build-support/release/rat_exclude_files.txt 7 files changed, 10,295 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/7604/8 -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build-support] added IWYU filter script
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7604 to look at the new patch set (#7). Change subject: [build-support] added IWYU filter script .. [build-support] added IWYU filter script Added a script to filter the output from the include-what-you-use tool. The idea is that the IWYU tool v0.8 is of alpha quality and produces many incorrect recommendations. Most of those can be addressed by using various 'IWYU pragma' directives, but not in the case of auto-generated files. Adding those directives is a very tedious job, and only some of those places are addressed. Moreover, the incorrect IWYU recommendations seem to be a little bit different version from version. Also, added IWYU mappings for the boost library (imported from the IWYU source tree) and the glog, gflags, gtest libraries. Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa --- A build-support/iwyu/iwyu-filter.awk A build-support/iwyu/mappings/boost-all-private.imp A build-support/iwyu/mappings/boost-all.imp A build-support/iwyu/mappings/gflags.imp A build-support/iwyu/mappings/glog.imp A build-support/iwyu/mappings/gtest.imp M build-support/release/rat_exclude_files.txt 7 files changed, 10,296 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/7604/7 -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build-support] added IWYU filter script
Alexey Serbin has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 6: (1 comment) I moved the IWYU license into the header of corresponding boost mapping files. http://gerrit.cloudera.org:8080/#/c/7604/6/build-support/iwyu/mappings/gflags.imp File build-support/iwyu/mappings/gflags.imp: Line 1: # > Nit: don't need the empty line at the beginning of the license. Done -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Adar Dembo has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/7604/6/build-support/iwyu/mappings/gflags.imp File build-support/iwyu/mappings/gflags.imp: Line 1: # Nit: don't need the empty line at the beginning of the license. Same goes for the other license inclusions. -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Alexey Serbin has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 6: (4 comments) > (4 comments) > > For the imported mapping files: > - Please add a file-level comment pointing to the source URL (if > applicable). > - Please add the appropriate license. Is it Apache-compatible? > > For mapping files you wrote: > - Add ASL copyright headers as well as file-level comments > explaining the file's purpose. Thank you for the reminder! The license the IWYU comes is a BSD-style one: https://github.com/include-what-you-use/include-what-you-use/blob/master/LICENSE.TXT I added the license file for boost mappings into the directory -- I thought that adding just a link to that file would not be enough. http://gerrit.cloudera.org:8080/#/c/7604/2/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: Line 39: # > I see. Maybe that merits an additional comment about IWYU being a static an It's a good idea -- I'll add that notion. http://gerrit.cloudera.org:8080/#/c/7604/5/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: PS5, Line 20: IWYU is of alpha quality and > IWYU Done PS5, Line 25: map > don't need Done PS5, Line 28: IWYU) and a f > just IWYU would be enough, I think. Done -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7604 to look at the new patch set (#6). Change subject: [build-support] added IWYU filter script .. [build-support] added IWYU filter script Added a script to filter the output from the include-what-you-use tool. The idea is that the IWYU tool v0.8 is of alpha quality and produces many incorrect recommendations. Most of those can be addressed by using various 'IWYU pragma' directives, but not in the case of auto-generated files. Adding those directives is a very tedious job, and only some of those places are addressed. Moreover, the incorrect IWYU recommendations seem to be a little bit different version from version. Also, added IWYU mappings for the boost library (imported from the IWYU source tree) and the glog, gflags, gtest libraries. Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa --- A build-support/iwyu/iwyu-filter.awk A build-support/iwyu/mappings/boost-all-private.imp A build-support/iwyu/mappings/boost-all.imp A build-support/iwyu/mappings/boost-mappings-LICENSE.TXT A build-support/iwyu/mappings/gflags.imp A build-support/iwyu/mappings/glog.imp A build-support/iwyu/mappings/gtest.imp M build-support/release/rat_exclude_files.txt 8 files changed, 10,274 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/7604/6 -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] separate DataDirManager from BlockManagers
Adar Dembo has posted comments on this change. Change subject: separate DataDirManager from BlockManagers .. Patch Set 6: (34 comments) http://gerrit.cloudera.org:8080/#/c/7602/6//COMMIT_MSG Commit Message: PS6, Line 23: directory directly http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: PS6, Line 205: // Pass in a report to prevent the block manager from logging : // unnecessarily. : FsReport report; Nit: move this down to just below Open(). Line 691: ASSERT_OK(env_util::CreateDirIfMissing(this->env_, paths[i])); Why not a simple CreateDir() here and below? Under what circumstances would this path already be created? http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS6, Line 260: // Exposes the underlying DataDirManager. : virtual DataDirManager* dd_manager() = 0; Why does this still exist? Shouldn't callers outside the block managers get the DataDirManager via the FsManager now? http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 65: LOG(INFO) << GetDirNames(kNumDirs)[0]; Still want this? Line 77: CHECK_OK(env_util::CreateDirIfMissing(env_, ret[i])); Why not a simpler CreateDir? http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 90: TAG_FLAG(fs_lock_data_dirs, unsafe); Also tag as 'evolving', for completeness. Line 272: const char* kInvalidPath = ""; Where is this used? PS6, Line 288: if (metric_entity) { : metrics_.reset(new DataDirMetrics(metric_entity)); : } You changed metric_entity to be pass-by-cref because it's not being moved here, right? How about adding an std::move here instead? Line 334: data_root_map_.swap(data_root_map); Maybe write to the member at the end of the function, after all the intermediate state has been assembled? Line 340: if (!canonicalized_data_fs_root.empty()) { When is this empty? Line 372: // Ensure the data dirs exist and create the instance files. IMHO this comment was better placed just before the for loop, so it's clear that it applies to the _entirety_ of the loop rather than the first few lines. Line 375: // In test, this is the input path, IRL, this is the paths with kDataDirName Please reword; not exactly sure what "input path" means, this should be more specific regarding kDataDirName (i.e. paths with it _appended_), and should say "In production" rather than IRL. Line 526: group_by_tablet_map_.clear(); Isn't this empty at Open() time anyway? Why bother? http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS6, Line 195: static const int kFlagCreateTestHolePunch = 0x1; : static const int kFlagCreateFsync = 0x2; These were only used in Create(), so they needn't exist at all anymore. Line 198: static const char* kDataDirName; Why does this need to be public? Doc it. Line 220: AccessMode mode = AccessMode::READ_WRITE); Can we get by without the default value? There aren't that many constructor calls, are there? Line 240: // max allowed, or if 'mode' is MANDATORY and locks could not be taken. 'mode' refers to a parameter that no longer exists. PS6, Line 229: // Initializes, sanitizes, and canonicalizes the directories. : Status Init(); : : // Initializes the data directories on disk. : // : // Returns an error if initialized directories already exist. : Status Create(); : : // Opens existing data roots from disk and indexes the files found. : // : // Returns an error if the number of on-disk directories found exceeds the : // max allowed, or if 'mode' is MANDATORY and locks could not be taken. : Status Open(); Normally we try to provide at most one, sometimes two functions with which to construct an object: 1. Just one: public constructor. For simple objects that can be either stack or heap allocated. Initialization cannot fail (since constructors do not return anything and our style guide forbids throwing exceptions). 2. Two: constructor and separate initializer. Sometimes the constructor is private, in which case the initializer is static and returns a heap allocated object. The initializer usually returns Status to indicate failures. We use this pattern ("two-phase initialization") when something in initialization may fail and we want to expose that. In rare cases, we apply pattern #2 with two different branches: one branch for creating something new on disk, and another for loading an existing something from disk. All this i
[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric
Adar Dembo has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric .. Patch Set 10: Code-Review+1 (1 comment) Todd and/or Mike should take another look. http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 385: status_pb_out->set_estimated_on_disk_size(tablet_size); > I'll take a local ref. PS10 seems to have reverted this back to "tablet size without cmeta size". Was that intentional? -- To view, visit http://gerrit.cloudera.org:8080/6968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Add tablet state summary metrics and fix KUDU-2044
Adar Dembo has posted comments on this change. Change subject: Add tablet state summary metrics and fix KUDU-2044 .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7618/2//COMMIT_MSG Commit Message: PS2, Line 11: The numbers are computed by the heartbeater. There's two reasons : for this: : 1. the heartbeater was already computing the number of RUNNING : and BOOTSTRAPPING tablets by holding the appropriate lock and : iterating over the tablet map : 2. the alternative to having some thread periodically iterate : over the tablet map is to increment and decrement the metrics : when tablets transition states. This is error-prone, : particularly if new states are added, and mistakes will : accumulate until the metric is worse than useless. > I could use a function gauge, but a really fast metric poll could contend w I'm not that worried about missing some places where calls to TransitionFromFooToBar() should go. What I don't like, though, is the additional plumbing that would require. It'd mean a backpointer to the TsTabletManager (with some hack for the master where there is no TsTabletManager), or a more abstract "notify tablet state change observers" pattern. How about this for a middle ground: 1. TsTabletManager maintains a timestamp representing the last time that the state metrics were calculated and a set of counters, one for each state. The timestamp could have a dedicated spinlock, or reuse an existing one if appropriate. 2. There are N function guages, one for each state. 3. When a guage is invoked, the timestamp is compared with the current time. If the delta exceeds some threshold (statically defined or configurable via gflag), the tablet map is walked and all of the counters are updated. If it doesn't, the current counter values are returned as-is. PS2, Line 23: A metric entity can now be : marked as hidden, so it will not print to /metrics, and : tablet metric entities are marked as hidden when the tablet is : tombstoned, and un-hidden if and when the tablet is revived. > They take up some memory as part of the TabletReplica that remains in the t Let's solicit another opinion first. Todd understands the metrics subsystem better than I do, and Mike can comment on the tombstoning/vivification lifecycle. -- To view, visit http://gerrit.cloudera.org:8080/7618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c82987ffe4f37e8af95972bde97841e44c521d9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] disk failure: add persistent disk states
Andrew Wong has posted comments on this change. Change subject: disk failure: add persistent disk states .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/7270/11/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: Line 26: #include "kudu/fs/fs.pb.h" > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/7270/11/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 30: #include "kudu/gutil/map-util.h" > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] disk failure: add persistent disk states
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7270 to look at the new patch set (#12). Change subject: disk failure: add persistent disk states .. disk failure: add persistent disk states This patch adds a new version of the path set so failed disks are not used the next time the server is run. This is needed, as data on a failed disk may be partially written and thus should not be used. To accomplish this, disk states, path names, and a timestamp are added to the path set. Additionally, if any disks are failed when loading instances from disk, an 'unhealthy' instance with no path set metadata will be created instead of failing the startup process. CheckIntegrity() is updated to accomodate this. Rather than comparing all instances against an agreed-upon set of UUIDs, the single path set with the largest timestamp is used as the main one to compare against (if only old path sets are available, the first healthy one is used). If no instances are healthy, CheckIntegrity() will fail, as all disks are failed. The main path set is checked to ensure its integrity (e.g. no duplicates), after which it is upgraded with the extra metadata if needed. It is then checked to ensure it is consistent with the healthy instances (e.g. having the right UUIDs and paths). Testing is done in a new iteration of CheckIntegrity(). Further testing is done in data_dirs-test to ensure the directory manager can be opened with failed disks. Some notes: - In the case of a server restart where all healthy disks fail to start up and some known failed disks start working again, the server will successfully start up with the bad disks (may have partially-written data). - If there are any unhealthy instances when upgrading the path sets (i.e. adding disk states, paths, timestamp), a complete mapping of UUIDs to paths will not be available, and CheckIntegrity() will fail. - The main path set's disk states are updated to reflect the failure of the instances. Since data on a failed disk cannot be trusted, disks that are successfully read from but are already marked FAILED by the main path set are not marked HEALTHY. This patch is a part of a series of patches to handle disk failures. To see how this fits, see 2.6 in: https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit?usp=sharing Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f --- M src/kudu/fs/block_manager_util-test.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/block_manager_util.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/fs.proto M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 9 files changed, 958 insertions(+), 197 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/12 -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Run the gradle build as a part of the gerrit tests
Adar Dembo has posted comments on this change. Change subject: Run the gradle build as a part of the gerrit tests .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7651/5/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: Line 56: # BUILD_JAVADefault: 1 > I think eventually we would switch to either maven or gradle and not contin Sure, I'm fine with that too. -- To view, visit http://gerrit.cloudera.org:8080/7651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1430cbd05ff78a69f2439e3a8f90e1ddde83a8d7 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [build-support] added IWYU filter script
Adar Dembo has posted comments on this change. Change subject: [build-support] added IWYU filter script .. Patch Set 5: (4 comments) For the imported mapping files: - Please add a file-level comment pointing to the source URL (if applicable). - Please add the appropriate license. Is it Apache-compatible? For mapping files you wrote: - Add ASL copyright headers as well as file-level comments explaining the file's purpose. http://gerrit.cloudera.org:8080/#/c/7604/2/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: Line 39: # CC=../../thirdparty/clang-toolchain/bin/clang > That was point of my concern -- in some places we use NDEBUG directives, an I see. Maybe that merits an additional comment about IWYU being a static analysis tool and thus only covering whatever code is enabled for the chosen build type? http://gerrit.cloudera.org:8080/#/c/7604/5/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: PS5, Line 20: the include-what-you-use tool IWYU PS5, Line 25: the don't need PS5, Line 28: the IWYU tool just IWYU would be enough, I think. -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [thirdparty]: added include-what-you-use
Alexey Serbin has posted comments on this change. Change subject: [thirdparty]: added include-what-you-use .. Patch Set 6: Verified+1 Unrelated flake in org.apache.kudu.client.TestScannerMultiTablet.org.apache.kudu.client.TestScannerMultiTablet -- To view, visit http://gerrit.cloudera.org:8080/7593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [thirdparty]: added include-what-you-use
Adar Dembo has posted comments on this change. Change subject: [thirdparty]: added include-what-you-use .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] WIP: [iwyu] first pass
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4738 to look at the new patch set (#14). Change subject: WIP: [iwyu] first pass .. WIP: [iwyu] first pass Updated C++ source files in accordance with include-what-you-use recommendations: * remove unused header files * add missing header files * use forward declarations when possible As a result, time of compilation reduced ~10% if building with GNU make in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com: prior: real 1m21.858s user 33m26.685s sys 2m19.831s after: real 1m12.686s user 30m26.891s sys 2m7.627s The next step is automating the checks, so the IWYU check would run automatically (like the LINT build). Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa --- M src/kudu/benchmarks/rle.cc M src/kudu/benchmarks/tpch/line_item_tsv_importer.h M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.h M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/benchmarks/wal_hiccup.cc M src/kudu/cfile/binary_dict_block.cc M src/kudu/cfile/binary_dict_block.h M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h M src/kudu/cfile/binary_prefix_block.cc M src/kudu/cfile/binary_prefix_block.h M src/kudu/cfile/block_cache-test.cc M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/block_compression.cc M src/kudu/cfile/block_compression.h M src/kudu/cfile/bloomfile-test-base.h M src/kudu/cfile/bloomfile-test.cc M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/bshuf_block.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.cc M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/index-test.cc M src/kudu/cfile/index_block.cc M src/kudu/cfile/index_block.h M src/kudu/cfile/index_btree.cc M src/kudu/cfile/index_btree.h M src/kudu/cfile/mt-bloomfile-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h M src/kudu/client/batcher.cc M src/kudu/client/batcher.h M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h M src/kudu/client/error-internal.cc M src/kudu/client/error-internal.h M src/kudu/client/error_collector.cc M src/kudu/client/error_collector.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/partitioner-internal.cc M src/kudu/client/partitioner-internal.h M src/kudu/client/predicate-test.cc M src/kudu/client/replica-internal.cc M src/kudu/client/replica-internal.h M src/kudu/client/resource_metrics.cc M src/kudu/client/resource_metrics.h M src/kudu/client/samples/sample.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scan_token-internal.h M src/kudu/client/scan_token-test.cc M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/session-internal.cc M src/kudu/client/session-internal.h M src/kudu/client/shared_ptr.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.cc M src/kudu/client/table_creator-internal.h M src/kudu/client/tablet-internal.h M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/clock/clock.h M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/hybrid_clock.cc M src/kudu/clock/hybrid_clock.h M src/kudu/clock/logical_clock-test.cc M src/kudu/clock/logical_clock.cc M src/kudu/clock/logical_clock.h M src/kudu/clock/mock_ntp.cc M src/kudu/clock/mock_ntp.h M src/kudu/clock/system_ntp.cc M src/kudu/clock/system_ntp.h M src/kudu/clock/system_unsync_time.cc M src/kudu/clock/system_unsync_time.h M src/kudu/codegen/code_cache.cc M src/kudu/codegen/code_cache.h M src/kudu/codegen/code_generator.cc M src/kudu/codegen/code_generator.h M src/kudu/codegen/codegen-test.
[kudu-CR] [thirdparty]: added include-what-you-use
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7593 to look at the new patch set (#6). Change subject: [thirdparty]: added include-what-you-use .. [thirdparty]: added include-what-you-use Build the include-what-you-use utility along with the LLVM toolchain. The project URL: https://include-what-you-use.org/ Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 --- M thirdparty/download-thirdparty.sh A thirdparty/patches/llvm-add-iwyu.patch A thirdparty/patches/llvm-iwyu-include-picker.patch A thirdparty/patches/llvm-iwyu-nocurses.patch M thirdparty/vars.sh 5 files changed, 39 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/6 -- To view, visit http://gerrit.cloudera.org:8080/7593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add tablet state summary metrics and fix KUDU-2044
Will Berkeley has posted comments on this change. Change subject: Add tablet state summary metrics and fix KUDU-2044 .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7618/2//COMMIT_MSG Commit Message: PS2, Line 11: The numbers are computed by the heartbeater. There's two reasons : for this: : 1. the heartbeater was already computing the number of RUNNING : and BOOTSTRAPPING tablets by holding the appropriate lock and : iterating over the tablet map : 2. the alternative to having some thread periodically iterate : over the tablet map is to increment and decrement the metrics : when tablets transition states. This is error-prone, : particularly if new states are added, and mistakes will : accumulate until the metric is worse than useless. > I appreciate the justification, but I don't think the heartbeater is a grea I could use a function gauge, but a really fast metric poll could contend with other access to the tablet manager's tablet map: I would need n function gauges for n tablet states, which is n locks and iterations each time someone hits /metrics. That's why I liked the heartbeat approach despite the drawbacks...it just adapts the iteration over the tablet map that was already done for heartbeats (which already iterated k times for k masters). I don't think multiply-instantiating a metrics has bad effects. It doesn't create multiple metrics. Is the alternative you support to track the state transitions directly? I.e. in TransitionFromFooToBar() do num_foos->IncrementBy(-1); num_bars->Increment(); PS2, Line 23: A metric entity can now be : marked as hidden, so it will not print to /metrics, and : tablet metric entities are marked as hidden when the tablet is : tombstoned, and un-hidden if and when the tablet is revived. > Why hide them and not remove them outright? Don't "hidden" entities still a They take up some memory as part of the TabletReplica that remains in the tablet manager's map, and they need to be iterated over and skipped when metrics are gathered. I think that's small overhead. The benefit of making them hidden is that it's very simple. Actually removing an entity means 1. removing all references to it 2. removing all reference to its child metrics 3. Hitting /metrics after the retirement period has passed (default 2 minutes) I think only the Tablet actually holds a reference to its entity, so 1 is easy. 2 is harder, because a number of other objects hold references to tablet metrics, e.g. the transaction tracker and log. Some are destroyed when the TabletReplica is shut down, for example the Tablet itself, and some aren't, e.g. the log and the transaction tracker. But, if you do think it's worth it to remove and then re-init the metrics in the various places when a tablet is tombstoned and then revived, I can do that. -- To view, visit http://gerrit.cloudera.org:8080/7618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c82987ffe4f37e8af95972bde97841e44c521d9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] WIP: [iwyu] first pass
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4738 to look at the new patch set (#13). Change subject: WIP: [iwyu] first pass .. WIP: [iwyu] first pass Updated C++ source files in accordance with include-what-you-use recommendations: * remove unused header files * add missing header files * use forward declarations when possible As a result, time of compilation reduced ~10% if building with GNU make in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com: prior: real 1m21.858s user 33m26.685s sys 2m19.831s after: real 1m12.686s user 30m26.891s sys 2m7.627s The next step is automating the checks, so the IWYU check would run automatically (like the LINT build). Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa --- M src/kudu/benchmarks/rle.cc M src/kudu/benchmarks/tpch/line_item_tsv_importer.h M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.h M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/benchmarks/wal_hiccup.cc M src/kudu/cfile/binary_dict_block.cc M src/kudu/cfile/binary_dict_block.h M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h M src/kudu/cfile/binary_prefix_block.cc M src/kudu/cfile/binary_prefix_block.h M src/kudu/cfile/block_cache-test.cc M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/block_compression.cc M src/kudu/cfile/block_compression.h M src/kudu/cfile/bloomfile-test-base.h M src/kudu/cfile/bloomfile-test.cc M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/bshuf_block.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.cc M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/index-test.cc M src/kudu/cfile/index_block.cc M src/kudu/cfile/index_block.h M src/kudu/cfile/index_btree.cc M src/kudu/cfile/index_btree.h M src/kudu/cfile/mt-bloomfile-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h M src/kudu/client/batcher.cc M src/kudu/client/batcher.h M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h M src/kudu/client/error-internal.cc M src/kudu/client/error-internal.h M src/kudu/client/error_collector.cc M src/kudu/client/error_collector.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/partitioner-internal.cc M src/kudu/client/partitioner-internal.h M src/kudu/client/predicate-test.cc M src/kudu/client/replica-internal.cc M src/kudu/client/replica-internal.h M src/kudu/client/resource_metrics.cc M src/kudu/client/resource_metrics.h M src/kudu/client/samples/sample.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scan_token-internal.h M src/kudu/client/scan_token-test.cc M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/session-internal.cc M src/kudu/client/session-internal.h M src/kudu/client/shared_ptr.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.cc M src/kudu/client/table_creator-internal.h M src/kudu/client/tablet-internal.h M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/clock/clock.h M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/hybrid_clock.cc M src/kudu/clock/hybrid_clock.h M src/kudu/clock/logical_clock-test.cc M src/kudu/clock/logical_clock.cc M src/kudu/clock/logical_clock.h M src/kudu/clock/mock_ntp.cc M src/kudu/clock/mock_ntp.h M src/kudu/clock/system_ntp.cc M src/kudu/clock/system_ntp.h M src/kudu/clock/system_unsync_time.cc M src/kudu/clock/system_unsync_time.h M src/kudu/codegen/code_cache.cc M src/kudu/codegen/code_cache.h M src/kudu/codegen/code_generator.cc M src/kudu/codegen/code_generator.h M src/kudu/codegen/codegen-test.
[kudu-CR] [build-support] added IWYU filter script
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7604 to look at the new patch set (#4). Change subject: [build-support] added IWYU filter script .. [build-support] added IWYU filter script Added a script to filter the output from the include-what-you-use tool. The idea is that the IWYU tool v0.8 is of alpha quality and produces many incorrect recommendations. Most of those can be addressed by using various 'IWYU pragma' directives, but not in the case of auto-generated files. Adding those directives is a very tedious job, and only some of those places are addressed. Moreover, the incorrect IWYU recommendations seem to be a little bit different version from version. Also, added IWYU mappings for the boost library (imported from the IWYU source tree) and the glog, gflags, gtest libraries. Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa --- A build-support/iwyu/iwyu-filter.awk A build-support/iwyu/mappings/boost-all-private.imp A build-support/iwyu/mappings/boost-all.imp A build-support/iwyu/mappings/gflags.imp A build-support/iwyu/mappings/glog.imp A build-support/iwyu/mappings/gtest.imp 6 files changed, 10,138 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/7604/4 -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP: [iwyu] first pass
Alexey Serbin has posted comments on this change. Change subject: WIP: [iwyu] first pass .. Patch Set 12: (21 comments) http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/rle.cc File src/kudu/benchmarks/rle.cc: Line 28: #include > Might be good to make a comment in the commit message about these different Good idea. I hope keeping these pragmas is a temporary solution until some bugs in the include-what-you-use tool are fixed or we find some other way of fixing it. http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/line_item_tsv_importer.h File src/kudu/benchmarks/tpch/line_item_tsv_importer.h: Line 21: #include > leftover? Done http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc File src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc: Line 18: #include > sys/types.h and gutil/port.h seem suspicious to me. I don't see anything i Yep, that's one of those recommendation the IWYU tools gave. As I already mentioned -- it's necessary to validate its suggestions for every time, and that takes a lot of time. Here it suggested to include for int64_t. Apparently, it should have been or instead. Line 18: #include > Perhaps sys/types.h is for the int64_t type? Right. Now it's fixed. http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao.h File src/kudu/benchmarks/tpch/rpc_line_item_dao.h: Line 24: #include "kudu/client/client.h" > Could you forward declare boost::function instead? Good idea. However, IWYU suggested this: kudu/src/kudu/benchmarks/tpch/rpc_line_item_dao.h should add these lines: #include // for function namespace boost { template class function; } Adding only the forward declaration would be enough. But if so, then IWYU complained continued to complain and I decided to just add . Also, it seems the previous version of the IWYU tools gave a different suggestions, not matching with those that current does (current is of v0.8). I'll update this to keep the forward declaration and adding a pragma for not suggesting to include http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch-schemas.h File src/kudu/benchmarks/tpch/tpch-schemas.h: Line 25: #include "kudu/client/schema.h" > Perhaps we could add this include inside the '#ifdef KUDU_HEADERS_NO_STUBS' Yep, this is kind of strange left-over from my earlier experiments, and it seems I forgot to address it. Thank you for pointing at it this time as well -- I'll address this. http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch1.cc File src/kudu/benchmarks/tpch/tpch1.cc: Line 60: #include > Another un-obvious one. That was a suggestion from IWYU for int32_t. Apparently, it should be covered by . I'll check what the current 0.8 version suggests. Line 60: #include > Probably int32_t (see struct Result). yep, that's true http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/scan_predicate.cc File src/kudu/client/scan_predicate.cc: Line 48: > extra Done http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: Line 24: #include > Hm, did plain boost/optional.hpp result in a warning? Yes, it did. That's why I updated this. We can amend this using our own mappings for the boost library. Right now I'm using the 'standard' mappings from the IWYU source tree. http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_creator-internal.h File src/kudu/client/table_creator-internal.h: Line 24: #include > I think this could be optional_fwd.hpp The class has a member of boost::optional type, so I don't think forward declaration would be feasible here. Yep -- the compiler output an error if trying to use the forward declaration. IWYU tool suggested to use , which seems good enough. http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/write_op.h File src/kudu/client/write_op.h: Line 20: // NOTE: using stdint.h instead of cstdint because this file is supposed > client_samples-test covers this, right? Probably no need to comment this a Done http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/clock/logical_clock.cc File src/kudu/clock/logical_clock.cc: Line 73: const MonoTime&) { > Don't we usually do something like /* deadline */? Done http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/codegen/row_projector.h File src/kudu/codegen/row_projector.h: Line 35: #include "kudu/gutil/ref_counted.h" > These keep showing up over and over, does that indicate some kind of issue I think this manifests a bug in the include-what-you-use tool, not with our header files organization. It's more apparent when you see the suggestions that IWYU outputs: /row_projector.h should add these lines: ... #include "kudu/gutil/int128.h" // for ostream ..
[kudu-CR] [WIP] Add BlockDeletionTransaction to Block Manager
Dan Burkert has posted comments on this change. Change subject: [WIP] Add BlockDeletionTransaction to Block Manager .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/7656/2/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 182: virtual void MarkDeletions(std::vector blocks, I think the docs on the 'deleted' parameter could use some work here - I'm not sure at a glance exactly what the out parameter should be used for. Line 273: virtual void CreateDeletionTransaction( Since this is infallible (doesn't return Status), could it return the scoped_refptr<..> instead of taking it as an out param? http://gerrit.cloudera.org:8080/#/c/7656/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1184: break; Not continue? Line 1217: std::vector blocks, can drop the std:: prefix Line 1225: WARN_NOT_OK(s, Substitute("Could not delete block $0", lowercase first letter Line 1983: make_scoped_refptr(new LogBlockDeletionTransaction(this))); consider using CreateDeletionTransaction PS2, Line 2173: C lowercase first letter -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Dan Burkert has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: Line 396: Status FileWritableBlock::FlushAndFinalize(FlushMode mode) { > Good catch, thanks! Do you think it is better to remove 'FLAGS_block_manage Good question, it does seem a bit confused to have the two flags. I'm not entirely sure what we're using the two for at the moment, I'd want Adar to weigh in. http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 278: Status FinishBlock(const Status& s, WritableBlock* block, > Thanks for pointing out! I personally prefer to keep it as the way it is, OK sounds good to me. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] disk failure: add persistent disk states
Andrew Wong has posted comments on this change. Change subject: disk failure: add persistent disk states .. Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/7270/10/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: Line 26: #include "kudu/fs/fs_manager.h" > warning: #includes are not sorted properly [llvm-include-order] Done Line 292: const string uuid = pb_at_i.uuid(); > warning: the const qualified variable 'uuid' is copy-constructed from a con Done http://gerrit.cloudera.org:8080/#/c/7270/10/src/kudu/fs/block_manager_util.h File src/kudu/fs/block_manager_util.h: Line 107: static Status CheckIntegrity(const std::vector& instances, > warning: function 'kudu::fs::PathInstanceMetadataFile::CheckIntegrity' has Done http://gerrit.cloudera.org:8080/#/c/7270/10/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 22: #include > warning: #includes are not sorted properly [llvm-include-order] Done Line 30: #include "kudu/gutil/strings/substitute.h" > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] disk failure: add persistent disk states
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7270 to look at the new patch set (#11). Change subject: disk failure: add persistent disk states .. disk failure: add persistent disk states This patch adds a new version of the path set so failed disks are not used the next time the server is run. This is needed, as data on a failed disk may be partially written and thus should not be used. To accomplish this, disk states, path names, and a timestamp are added to the path set. Additionally, if any disks are failed when loading instances from disk, an 'unhealthy' instance with no path set metadata will be created instead of failing the startup process. CheckIntegrity() is updated to accomodate this. Rather than comparing all instances against an agreed-upon set of UUIDs, the single path set with the largest timestamp is used as the main one to compare against (if only old path sets are available, the first healthy one is used). If no instances are healthy, CheckIntegrity() will fail, as all disks are failed. The main path set is checked to ensure its integrity (e.g. no duplicates), after which it is upgraded with the extra metadata if needed. It is then checked to ensure it is consistent with the healthy instances (e.g. having the right UUIDs and paths). Testing is done in a new iteration of CheckIntegrity(). Further testing is done in data_dirs-test to ensure the directory manager can be opened with failed disks. Some notes: - In the case of a server restart where all healthy disks fail to start up and some known failed disks start working again, the server will successfully start up with the bad disks (may have partially-written data). - If there are any unhealthy instances when upgrading the path sets (i.e. adding disk states, paths, timestamp), a complete mapping of UUIDs to paths will not be available, and CheckIntegrity() will fail. - The main path set's disk states are updated to reflect the failure of the instances. Since data on a failed disk cannot be trusted, disks that are successfully read from but are already marked FAILED by the main path set are not marked HEALTHY. This patch is a part of a series of patches to handle disk failures. To see how this fits, see 2.6 in: https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit?usp=sharing Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f --- M src/kudu/fs/block_manager_util-test.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/block_manager_util.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/fs.proto M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 9 files changed, 958 insertions(+), 196 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/11 -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] disk failure: add persistent disk states
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7270 to look at the new patch set (#10). Change subject: disk failure: add persistent disk states .. disk failure: add persistent disk states This patch adds a new version of the path set so failed disks are not used the next time the server is run. This is needed, as data on a failed disk may be partially written and thus should not be used. To accomplish this, disk states, path names, and a timestamp are added to the path set. Additionally, if any disks are failed when loading instances from disk, an 'unhealthy' instance with no path set metadata will be created instead of failing the startup process. CheckIntegrity() is updated to accomodate this. Rather than comparing all instances against an agreed-upon set of UUIDs, the single path set with the largest timestamp is used as the main one to compare against (if only old path sets are available, the first healthy one is used). If no instances are healthy, CheckIntegrity() will fail, as all disks are failed. The main path set is checked to ensure its integrity (e.g. no duplicates), after which it is upgraded with the extra metadata if needed. It is then checked to ensure it is consistent with the healthy instances (e.g. having the right UUIDs and paths). Testing is done in a new iteration of CheckIntegrity(). Further testing is done in data_dirs-test to ensure the directory manager can be opened with failed disks. Some notes: - In the case of a server restart where all healthy disks fail to start up and some known failed disks start working again, the server will successfully start up with the bad disks (may have partially-written data). - If there are any unhealthy instances when upgrading the path sets (i.e. adding disk states, paths, timestamp), a complete mapping of UUIDs to paths will not be available, and CheckIntegrity() will fail. - The main path set's disk states are updated to reflect the failure of the instances. Since data on a failed disk cannot be trusted, disks that are successfully read from but are already marked FAILED by the main path set are not marked HEALTHY. This patch is a part of a series of patches to handle disk failures. To see how this fits, see 2.6 in: https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit?usp=sharing Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f --- M src/kudu/fs/block_manager_util-test.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/block_manager_util.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/fs.proto M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc 9 files changed, 958 insertions(+), 195 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/10 -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric
Will Berkeley has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric .. Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: Line 285: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update cmeta on-disk size"); > Could RETURN_NOT_OK here too. Done Line 322: WARN_NOT_OK(cmeta->UpdateOnDiskSize(), "Failed to update cmeta on-disk size"); > And here. Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: Line 241: uint64_t on_disk_size_; > Ah, I just read your earlier discussion with Mike. Nevermind. Right, class isn't threadsafe. http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: PS9, Line 331: // Return the on-disk size of this rowset's cfile set, including bloomfiles : // and the ad hoc index, in bytes. : uint64_t BaseDataOnDiskSize() const; : : // Return the size on-disk of this rowset's REDO deltas, in bytes. : uint64_t RedoDeltaOnDiskSize() const; : : // Return the size on-disk of this rowset, in bytes. : uint64_t OnDiskSize() const OVERRIDE; : : // Return the size on-disk of the data in this rowset (i.e. excluding metadata), in bytes. : uint64_t OnDiskDataSize() const OVERRIDE; > The number of parts is high; could you add a block comment just before thes Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: Line 115: virtual Status DebugDump(std::vector *lines = NULL) = 0; > warning: default arguments on virtual or override methods are prohibited [g Ignored. http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: Line 237: // Return the total on-disk size of this tablet, in bytes. > Without additional commenting, it seems like the value returned by OnDiskSi Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 309: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update superblock on-disk size"); > Seems reasonable to RETURN_NOT_OK here. Done Line 587: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update superblock on-disk size"); > Maybe even RETURN_NOT_OK here too. I think your fear is "if this fails, we' Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 87: "Space used by this tablet on disk."); > Maybe mention that this includes both data and metadata? Done Line 377: tablet_size = OnDiskSizeUnlocked(); > We add in cmeta_size below and this is the only use of OnDiskSizeUnlocked. Because it causes a potential deadlock-- getting the cmeta size requires taking a lock in consensus, so OnDiskSize would acquire the locks in the order tablet_replica lock -> consensus lock, but (IIRC) the transaction code acquires the locks in the order consensus lock -> tablet_replica lock. There was a TSAN failure showing this in the pre-commit jobs but it looks like the links are broken now :/ Line 385: size_t cmeta_size = consensus_ ? consensus_->MetadataOnDiskSize() : 0; > This isn't safe; you need to take a local ref of consensus_ and then test i I'll take a local ref. Line 640: size_t TabletReplica::OnDiskSize() const { > Normally the only distinction between Foo and FooUnlocked is the acquisitio Done Line 656: size_t TabletReplica::OnDiskSizeUnlocked() const { > Can you DCHECK that lock_ is held? Done -- To view, visit http://gerrit.cloudera.org:8080/6968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6968 to look at the new patch set (#10). Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric .. KUDU-1755 Part 2: Improve tablet on-disk size metric This adds WAL segment and cmeta sizes to the tablet on-disk metric. It also fixes the tablet superblock on disk size, which was being underestimated because the size was measured as the PB size instead of counting the container file the PB is stored in. It also exposes a second metric, Tablet Data Size On Disk, which measures just the size of data blocks and does not include metadata or WAL segments. Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/log-test.cc M src/kudu/consensus/log.cc M src/kudu/consensus/log.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_metadata-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h 23 files changed, 257 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/6968/10 -- To view, visit http://gerrit.cloudera.org:8080/6968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Run the gradle build as a part of the gerrit tests
Grant Henke has posted comments on this change. Change subject: Run the gradle build as a part of the gerrit tests .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/7651/5/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: Line 56: # BUILD_JAVADefault: 1 > Update this comment to reflect that this builds the Java sources with Maven I think eventually we would switch to either maven or gradle and not continue to have both since the maintenance of keeping them in sync is high. My though is to leave BUILD_JAVA generic and just have BUILD_GRADLE as an option until we swap completely. Line 78: # Java tests. > Hmm, but we're not using gradle to run tests, just to build. Is this intend I can change this. Mainly a copy paste error. Line 385: # Rerun the build using the Gradle build. > Can we break this out of BUILD_JAVA so that you could run build-and-test.sh See my other commend above. I think BUILD_JAVA should wrap BUILD_GRADLE since at some point we will remove maven and BUILD_JAVA will essentially become BUILD_GRADLE. -- To view, visit http://gerrit.cloudera.org:8080/7651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1430cbd05ff78a69f2439e3a8f90e1ddde83a8d7 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java] Update outdated dependencies
Grant Henke has posted comments on this change. Change subject: [java] Update outdated dependencies .. Patch Set 5: Yeah, they are absolutely legitimate test errors. It looks like another issue with shading order since it claims a method definition doesn't exist because its return class is changed when shaded. An upgrade to a maven plugin likely caused this. I may move the maven plugin updates to a different patch. -- To view, visit http://gerrit.cloudera.org:8080/7647 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] WIP: [iwyu] first pass
Alexey Serbin has uploaded a new patch set (#12). Change subject: WIP: [iwyu] first pass .. WIP: [iwyu] first pass Updated C++ source files in accordance with include-what-you-use recommendations: * remove unused header files * add missing header files * use forward declarations when possible As a result, time of compilation reduced ~10% if building with GNU make in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com: prior: real 1m21.858s user 33m26.685s sys 2m19.831s after: real 1m12.686s user 30m26.891s sys 2m7.627s The next step is automating the checks, so the IWYU check would run automatically (like the LINT build). Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa --- M src/kudu/benchmarks/rle.cc M src/kudu/benchmarks/tpch/line_item_tsv_importer.h M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc M src/kudu/benchmarks/tpch/rpc_line_item_dao.h M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/benchmarks/wal_hiccup.cc M src/kudu/cfile/binary_dict_block.cc M src/kudu/cfile/binary_dict_block.h M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h M src/kudu/cfile/binary_prefix_block.cc M src/kudu/cfile/binary_prefix_block.h M src/kudu/cfile/block_cache-test.cc M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/block_compression.cc M src/kudu/cfile/block_compression.h M src/kudu/cfile/bloomfile-test-base.h M src/kudu/cfile/bloomfile-test.cc M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/bshuf_block.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_util.cc M src/kudu/cfile/cfile_util.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/index-test.cc M src/kudu/cfile/index_block.cc M src/kudu/cfile/index_block.h M src/kudu/cfile/index_btree.cc M src/kudu/cfile/index_btree.h M src/kudu/cfile/mt-bloomfile-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h M src/kudu/client/batcher.cc M src/kudu/client/batcher.h M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h M src/kudu/client/error-internal.cc M src/kudu/client/error-internal.h M src/kudu/client/error_collector.cc M src/kudu/client/error_collector.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/partitioner-internal.cc M src/kudu/client/partitioner-internal.h M src/kudu/client/predicate-test.cc M src/kudu/client/replica-internal.cc M src/kudu/client/replica-internal.h M src/kudu/client/resource_metrics.cc M src/kudu/client/resource_metrics.h M src/kudu/client/samples/sample.cc M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_predicate.cc M src/kudu/client/scan_predicate.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scan_token-internal.h M src/kudu/client/scan_token-test.cc M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/session-internal.cc M src/kudu/client/session-internal.h M src/kudu/client/shared_ptr.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.cc M src/kudu/client/table_creator-internal.h M src/kudu/client/tablet-internal.h M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/client/write_op.cc M src/kudu/client/write_op.h M src/kudu/clock/clock.h M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/hybrid_clock.cc M src/kudu/clock/hybrid_clock.h M src/kudu/clock/logical_clock-test.cc M src/kudu/clock/logical_clock.cc M src/kudu/clock/logical_clock.h M src/kudu/clock/mock_ntp.cc M src/kudu/clock/mock_ntp.h M src/kudu/clock/system_ntp.cc M src/kudu/clock/system_ntp.h M src/kudu/clock/system_unsync_time.cc M src/kudu/clock/system_unsync_time.h M src/kudu/codegen/code_cache.cc M src/kudu/codegen/code_cache.h M src/kudu/codegen/code_generator.cc M src/kudu/codegen/code_generator.h M src/kudu/codegen/codegen-test.cc M src/kudu/codegen/compilation_manager.cc M src/kudu/codegen/compilation_manager.h M src/kudu/cod
[kudu-CR] [build-support] added IWYU filter script
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7604 to look at the new patch set (#3). Change subject: [build-support] added IWYU filter script .. [build-support] added IWYU filter script Added a script to filter the output from the include-what-you-use tool. The idea is that the IWYU tool v0.8 is of alpha quality and produces many incorrect recommendations. Most of those can be addressed by using various 'IWYU pragma' directives, but not in the case of auto-generated files. Adding those directives is a very tedious job, and only some of those places are addressed. Moreover, the incorrect IWYU recommendations seem to be a little bit different version from version. Also, added IWYU mappings for the boost library (imported from the IWYU source tree) and the glog, gflags, gtest libraries. Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa --- A build-support/iwyu/iwyu-filter.awk A build-support/iwyu/mappings/boost-all-private.imp A build-support/iwyu/mappings/boost-all.imp A build-support/iwyu/mappings/gflags.imp A build-support/iwyu/mappings/glog.imp A build-support/iwyu/mappings/gtest.imp 6 files changed, 10,137 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/7604/3 -- To view, visit http://gerrit.cloudera.org:8080/7604 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [thirdparty]: added include-what-you-use
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7593 to look at the new patch set (#4). Change subject: [thirdparty]: added include-what-you-use .. [thirdparty]: added include-what-you-use Build the include-what-you-use utility along with the LLVM toolchain. The project URL: https://include-what-you-use.org/ Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 --- M thirdparty/download-thirdparty.sh A thirdparty/patches/llvm-add-iwyu.patch A thirdparty/patches/llvm-iwyu-include-picker.patch A thirdparty/patches/llvm-iwyu-nocurses.patch M thirdparty/vars.sh 5 files changed, 47 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7593/4 -- To view, visit http://gerrit.cloudera.org:8080/7593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I041ad9e4bbff410f1760c7abd8cd173e5e9cc564 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon