[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

2017-08-14 Thread Todd Lipcon (Code Review) 
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

[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

2017-08-14 Thread Sailesh Mukil (Code Review) 
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

2017-08-14 Thread Sailesh Mukil (Code Review) 
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] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Andrew Wong (Code Review) 
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

2017-08-14 Thread Andrew Wong (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Adar Dembo (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

[kudu-CR] [thirdparty]: added include-what-you-use

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Will Berkeley (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

[kudu-CR] [build-support] added IWYU filter script

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Dan Burkert (Code Review) 
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

2017-08-14 Thread Dan Burkert (Code Review) 
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

2017-08-14 Thread Andrew Wong (Code Review) 
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

2017-08-14 Thread Andrew Wong (Code Review) 
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

2017-08-14 Thread Andrew Wong (Code Review) 
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

2017-08-14 Thread Will Berkeley (Code Review) 
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

2017-08-14 Thread Will Berkeley (Code Review) 
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

2017-08-14 Thread Grant Henke (Code Review) 
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

2017-08-14 Thread Grant Henke (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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

[kudu-CR] [build-support] added IWYU filter script

2017-08-14 Thread Alexey Serbin (Code Review) 
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

2017-08-14 Thread Alexey Serbin (Code Review) 
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