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

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [build-support] added IWYU filter script
..


Patch Set 2:

(12 comments)

Thank you for the review!  I'll post a new version as soon as I shorten the 
muted list enough as I'm working on https://gerrit.cloudera.org/#/c/4738/.  I 
think I'll mute the most of the tests and address the 'core' code.

That's to avoid needless rebases.

http://gerrit.cloudera.org:8080/#/c/7604/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

PS2, Line 19: include-what-you-use
> "include-what-you-use (IWYU) tool"
Done


PS2, Line 20: for
> of
Done


PS2, Line 21: amended
> Is "amended" the right word here? The pragmas silences certain IWYU recomme
In the end -- yes, it silences those.  It seems I was trying to say something 
else here.  But let's put it simple.


PS2, Line 24: # Also, it's possible to address the boost-related headers using 
special
: # mapping (the mapping needs to be implemented).
> Can you provide an example of what this is referring to?
It's already done, actually.  I'll remove this comment.

But for the reference, there is a mechanism of so called 'mappings':
  
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUMappings.md

The tool provides those boost mappings (maybe, a bit outdated):
  
https://github.com/include-what-you-use/include-what-you-use/blob/master/boost-all.imp


PS2, Line 29: For this, CMake version of 3.3 and higher is required.
> Doesn't Kudu require a higher version anyway? So maybe this recommendation 
It's a good point, I think it's better to remove this.


Line 34: # 
IWYU=/thirdparty/clang-toolchain/bin/include-what-you-use
> Why does this have to be an absolute path? You could make it absolute using
Because that path is put into the generated GNU makefiles.  I wish it were 
possible to use a relative one, but that didn't work.

Yes, that can be achieved using that command-line.  I just wanted to stress the 
path should be an absolute one.  I'll keep that wording and add the path 
specification using `pwd`, as suggested.  Thanks!


Line 39: #   -DCMAKE_BUILD_TYPE=debug \
> Isn't this the default value? Can be omitted?
That was point of my concern -- in some places we use NDEBUG directives, and it 
might be worth to run the tool in the RELEASE configuration.

But as for the example -- yes, it can be omitted.


PS2, Line 47: 
> Just replace with $(nproc)
Done


PS2, Line 56: to
> drop this
Done


PS2, Line 56: the
> and this
Done


Line 645:   in_ctx = 1
> I think I understand what in_ctx does; what does this do?
Oh, that's gone.  Will publish a new one soon.


PS2, Line 648: ~
> Isn't this a regex match? Why not a string comparison?
It's not exact match because the path in the IWYU output is the absolute one.  
So, here is the catch: I don't want to introduce the notion of the KUDU_ROOT in 
this script, because it would be necessary to pass it around, etc.  Otherwise, 
I would just use the key as is (which is much faster, especially if the number 
of elements in the 'muted' array is significant).

However, the 'muted' array should be very short or even empty further down the 
road.  So, I decided to go this way.

But it might be faster just to check for the 'suffix substring' relationship 
instead of generic regex, right.  I'll give it a try.


-- 
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: 2
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] Add tablet state summary metrics and fix KUDU-2044

[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:

(3 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 great 
place either, since there's one heartbeating thread per master. So your current 
approach instantiates each metric multiple times (not to mention the 
unnecessary updates incurred from having more than one  heartbeating thread).


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 add 
ever-growing load given how we do tombstoning?


http://gerrit.cloudera.org:8080/#/c/7618/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

Line 533:   std::atomic hidden_;
The rest of the file uses util/atomic.h for atomics; please do the same here.

Also, are hidden/set_hidden accessed often? If not, could also make this a 
simple bool and protect with lock_.


-- 
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] KUDU-1755 Part 2: Improve tablet on-disk size metric

[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 9:

(12 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.


Line 322:   WARN_NOT_OK(cmeta->UpdateOnDiskSize(), "Failed to update cmeta 
on-disk size");
And here.


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_;
Why doesn't this need to be atomic (like TabletReplica::on_disk_size_)?


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 these 
functions describing the different space-occupying parts of a DRS?


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 OnDiskSize() 
should include _all_ metadata. That is, not just the superblock but also the 
cmeta. But of course we can't do that since the cmeta isn't known here.

Can you update the comments (here and to OnDiskDataSize) to clarify this?


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.


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'll 
return failure even though we did replace the superblock". The counter-argument 
is: "WritePBContainerToPath can fail and still replace the superblock (i.e. a 
failure in SyncDir)".


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?


Line 377: tablet_size = OnDiskSizeUnlocked();
We add in cmeta_size below and this is the only use of OnDiskSizeUnlocked. Why 
not make OnDiskSizeUnlocked return the cmeta size too?


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 it 
for nullitude. Or you need to make the call with lock_ held, which guarantees 
that consensus_ isn't modified.

Okay, I went and read the comments in TabletReplica which talk about how 
consensus_ is a "set-once" object. That seems bogus to me, though admittedly 
I'm not an expert on this code. TabletReplica::Shutdown() resets consensus_ 
with lock_ held. Can we guarantee that when Shutdown() is called, no other 
thread (besides the one calling Shutdown()) will invoke a TabletReplica 
function? I don't see how we can (hence the fragility). And if we can't, then 
consensus_ access needs to be atomic, either by taking a local ref then 
operating on it, or by acquiring lock_ first.


Line 640: size_t TabletReplica::OnDiskSize() const {
Normally the only distinction between Foo and FooUnlocked is the acquisition of 
a lock. Here OnDiskSizeUnlocked does not include the consensus metadata size.

Can you rename the functions accordingly?


Line 656: size_t TabletReplica::OnDiskSizeUnlocked() const {
Can you DCHECK that lock_ is held?


-- 
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: 

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

[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 9:

(1 comment)

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_;
> Why doesn't this need to be atomic (like TabletReplica::on_disk_size_)?
Ah, I just read your earlier discussion with Mike. Nevermind.


-- 
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] [build-support] added IWYU filter script

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [build-support] added IWYU filter script
..


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7604/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

PS2, Line 19: include-what-you-use
"include-what-you-use (IWYU) tool"

Then you can use "IWYU" henceforth in the file instead of having to repeat the 
more cumbersome "include-what-you-use".


PS2, Line 20: for
of


PS2, Line 21: amended
Is "amended" the right word here? The pragmas silences certain IWYU 
recommendations, no?


PS2, Line 24: # Also, it's possible to address the boost-related headers using 
special
: # mapping (the mapping needs to be implemented).
Can you provide an example of what this is referring to?


PS2, Line 29: For this, CMake version of 3.3 and higher is required.
Doesn't Kudu require a higher version anyway? So maybe this recommendation 
doesn't really matter?


Line 34: # 
IWYU=/thirdparty/clang-toolchain/bin/include-what-you-use
Why does this have to be an absolute path? You could make it absolute using 
`pwd`/../../thirdparty/..., right?


Line 39: #   -DCMAKE_BUILD_TYPE=debug \
Isn't this the default value? Can be omitted?


PS2, Line 47: 
Just replace with $(nproc)


PS2, Line 56: to
drop this


PS2, Line 56: the
and this


Line 645:   in_ctx = 1
I think I understand what in_ctx does; what does this do?


PS2, Line 648: ~
Isn't this a regex match? Why not a string comparison?


-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [thirdparty]: added include-what-you-use
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/patches/llvm-iwyu-nocurses.patch
File thirdparty/patches/llvm-iwyu-nocurses.patch:

What does IWYU use curses for? What is the effect of removing it?


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [thirdparty]: added include-what-you-use
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7593/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS3, Line 264:   fetch_and_expand 
include-what-you-use-${IWYU_VERSION}.src.tar.gz \
 : ${LLVM_SOURCE}/tools/clang/tools
Right now updating LLVM involves extracting the LLVM tarball, adding clang and 
a bunch of other stuff, retarring it, and uploading the result. The new 
EXTRACT_SUBDIR functionality you added to fetch_and_expand means we could 
perhaps get away with uploading the various LLVM and clang tarballs to S3 and 
piecing the directory tree together on the client machine, like you did for 
IWYU.

In any case, adding IWYU like this represents an unusual middle ground: the 
LLVM tarball is not fully preprocessed, nor is it fully postprocessed. Could 
you either adhere to the status quo and upload a new LLVM tarball to S3 
containing IWYU, or redo this to fully componentize LLVM and piece the 
directory tree together locally?

Which approach is preferred may depend on whether IWYU needs to be 
patched/updated with every LLVM release. Is there some semblance of 
compatibility? Or is there a 1-1 relationship between each release?


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 11:

(2 comments)

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
Perhaps sys/types.h is for the int64_t type?


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.
Probably int32_t (see struct Result).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] python: restore setuptools requirement and employ different workaround

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: python: restore setuptools requirement and employ different 
workaround
..


Patch Set 1: Verified+1

One test failed in test setup:

 INFO  42520run_isolated(173): Running ['/usr/bin/python', 
u'../../build-support/run_dist_test.py', u'-e', u'GTEST_SHARD_INDEX=0', u'-e', 
u'GTEST_TOTAL_SHARDS=1', u'-e', u'KUDU_TEST_TIMEOUT=870', u'-e', 
u'KUDU_ALLOW_SLOW_TESTS=1', u'-e', u'KUDU_COMPRESS_TEST_OUTPUT=1', u'--', 
u'../release/bin/catalog_manager-test'], 
cwd=/tmp/run_tha_testXgd2YF/build/isolate
Traceback (most recent call last):
  File "../../build-support/run_dist_test.py", line 150, in 
main()
  File "../../build-support/run_dist_test.py", line 122, in main
fixup_rpaths(os.path.join(ROOT, "build"))
  File "../../build-support/run_dist_test.py", line 90, in fixup_rpaths
fix_rpath(p)
  File "../../build-support/run_dist_test.py", line 75, in fix_rpath
rpath = re.search("R(?:UN)?PATH=(.+)", stdout.strip()).group(1)
AttributeError: 'NoneType' object has no attribute 'group'

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a8947bc8e89ec73728749bbcc6e9b919e26838a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Update outdated dependencies

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 5:

Looks fine but the test failures look legit.

-- 
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] KUDU-2091: Certificates with intermediate CA's do not work with Kudu

[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 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

PS1, Line 162: CHECK_OK
ASSERT_OK() ?


PS1, Line 171: LOG(INFO) << "Connecting to " << server_addr.ToString();
Is this crucial to have this info line in the test?  As I understand, this 
might be useful only if test fails -- if really necessary, consider using 
SCOPED_TRACE() with the server_addr.ToString().


PS1, Line 178: for (int i = 0; i < 10; i++) {
nit: just curious why calling it once is not enough


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

Line 214: void Cert::AdoptAndAddRefRawData(STACK_OF(X509)* data) {
Why not just to use X509_chain_up_ref() and remove the constraint on 
data_chain_len?


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:

PS1, Line 76: STACK_OF(X509)
nit: this is RawDataType typedef


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, 
pem_password_cb* unused2,
> yea, I think this is better off in the .cc file anyway since it's too large
once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERRORS


PS1, Line 178:   STACK_OF(X509)* sk = sk_X509_new_null();
 :   if (sk_X509_push(sk, x) == 0) return nullptr;
There might be a memory leak if the 'sk' is allocated but xk_X509_push() 
failed.  Consider using ssl_make_unique() construct (you could look around for 
examples).


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:

Line 499: Status CreateTestSSLCertSignedByChain(const string& dir,
> add a comment explaining how this was generated?
nit: indentation for the last 3 parameters


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

PS1, Line 194:   if (cert) remote_cert_.AdoptX509(cert);
nit: consider keeping the original brace style here


-- 
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] Remove double brackets from wrapper script

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: [java] Remove double brackets from wrapper script
..


Patch Set 2:

(1 comment)

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

Line 13: and not a bash script. In that case [[ is not built in.
> FWIW, we do enforce that /bin/bash is the interpreter for all Kudu shell sc
This file is auto-generated by `gradle wrapper` and then we inject a few lines 
to support not checking a jar in. It's probably less risky not to change it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I839c47bbef8bf901047b9379be958f4cebbd406e
Gerrit-PatchSet: 2
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] Update outdated dependencies

[Grant Henke (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7647

to look at the new patch set (#5).

Change subject: [java] Update outdated dependencies
..

[java] Update outdated dependencies

Includes the following version updates:
- Avro 1.8.1 -> 1.8.2
- Flume 1.6.0 -> 1.7.0
- Gradle 4.0.1 -> 4.1.0
- Jepsen 0.1.3 -> 0.1.5
- Spark 2.1.1 -> 2.2.0
- Many build plugin versions

Also moves and sorts the versions alphabetically.

Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
---
M java/gradle/buildscript.gradle
M java/gradle/dependencies.gradle
M java/gradle/wrapper/gradle-wrapper.properties
M java/gradlew
M java/kudu-flume-sink/pom.xml
M java/kudu-jepsen/pom.xml
M java/kudu-spark-tools/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
9 files changed, 44 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/7647/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7647
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

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

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 
Kudu
..


Patch Set 1:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

Line 71: X509* Cert::GetEndOfChainCert() const {
worth a CHECK_GT(chain_len_, 0); or an if statement and return nullptr in the 
case of an empty stack.


PS1, Line 72: static_cast(sk_value
could you use sk_X509_value(data_.get(), chain_len_ - 1) here?


Line 78:   if (s.ok()) chain_len_ = sk_num(reinterpret_cast(data_.get()));
sk_X509_num?


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:

PS1, Line 63: this 
the end-user cert?


PS1, Line 89: GetEndOfChainCert
nit: think this should be named GetEndOfChainX509 since it returns an X509* 
instead of a Cert wrapper


Line 93:   int chain_len_ = 0;
why not just use sk_X509_num(data_.get()) where necessary? is caching the value 
necessary?


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, 
pem_password_cb* unused2,
> warning: function 'PEM_read_STACK_OF_X509' defined in a header file; functi
yea, I think this is better off in the .cc file anyway since it's too large to 
inline.


PS1, Line 131:   STACK_OF(X509_INFO)* info;
 :   info = PEM_X509_INFO_read_bio(bio, nullptr, nullptr, nullptr);
 :  
combine onto one line


Line 138: sk_X509_INFO_pop_free(info, X509_INFO_free);
can you use a ScopedCleanup to make this automatic right after allocating the 
'info' and avoid having it in three places?


PS1, Line 143: TACK_OF(X509)* sk;
 :   sk = sk_X509_new_null();
combine on one line?


Line 147:   X509_INFO *stack_item = nullptr;
move into the loop body since it's only used in that scope?


Line 162:   unsigned chain_len = sk_num(reinterpret_cast(obj));
sk_X509_num?
use 'int' instead of 'unsigned' to make unsigned overflow bugs less likely


Line 164:   X509* cert_item = nullptr;
move inside loop


PS1, Line 167: int ret;
 : ret = PEM_write_bio_X509(bio, cert_item);
 :  
combine

(it looks like a bunch of this code has this style - I'm guessing you borrowed 
it from some C language example. Is there any copyright notice we need to 
retain? is the license OK?)


Line 176:   // We don't support chain certificates written in DER format.
is such a thing actually commonplace out there? can we at least detect it and 
LOG(FATAL) or otherwise return some warning instead of silently ignoring?


Line 177:   X509* x = d2i_X509_bio(bio, nullptr);
what if x is null?


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:

Line 499: Status CreateTestSSLCertSignedByChain(const string& dir,
add a comment explaining how this was generated?


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.h
File src/kudu/security/test/test_certs.h:

Line 74:  std::string* cert_file,
nit: indentation


http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

Line 255: X509* inner_cert =
sk_X509_value


Line 270:   trusted_cert_count_ += 1;
should you add chain_len() here?


-- 
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow tablet shutdown without completing txs

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow tablet shutdown without completing txs
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7439/11//COMMIT_MSG
Commit Message:

Line 21: Testing is done by adding the following:
> forgot to ask: does TestDeleteTableWithConcurrentWrites in delete_table-ite
sorry for the email flood, but a couple more test suggestions:

- TestDeleteTableWithConcurrentWrites focuses on complete deletion of a table 
during a write workload, and expects the writes to start getting a NotFound 
error since the table is gone.
- We should add another which shuts down (eg by injecting a failure) just a 
single replica of a repl=3 table and ensures that the client does _not_ see any 
errors. ie it should see the "Aborted" errors and transparently fail over to 
another replica. It should then restart the server so that the tablet is 
available again and make sure that it can recover properly. Hopefully this test 
should be randomized enough to sometimes pick a leader, sometimes a follower.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow tablet shutdown without completing txs

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow tablet shutdown without completing txs
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7439/11//COMMIT_MSG
Commit Message:

Line 21: Testing is done by adding the following:
forgot to ask: does TestDeleteTableWithConcurrentWrites in delete_table-itest 
also cover this code pretty well? I think it's worth looping that one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
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

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7662

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.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
9 files changed, 424 insertions(+), 48 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[kudu-CR] python: restore setuptools requirement and employ different workaround

[Adar Dembo (Code Review)]

Hello David Ribeiro Alves, Jean-Daniel Cryans,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/7661

to review the following change.

Change subject: python: restore setuptools requirement and employ different 
workaround
..

python: restore setuptools requirement and employ different workaround

Commit d0acb55 moved the installation of setuptools from requirements.txt
to build-and-test.sh itself. This is somewhat inconvenient, and it reduces
the usefulness of requirements.txt; it's no longer a "one stop shop" for all
dependencies needed to build the Python bindings.

The build problems we saw were due to a virtualenv like so:
1. Initially, an old pip and setuptools
2. An upgraded pip (via `pip install --upgrade pip` in build-and-test.sh)

When `pip install -r requirements.txt` is run with such a virtualenv, the
new pip tries to upgrade the old setuptools and is unable to do so.

Let's try a different approach: let's not upgrade pip at all. We'll start
with whatever pip/setuptools are in the virtualenv, and we'll use that pip
to upgrade setuptools via the usual `pip install -r requirements.txt`. This
appears to work on both CentOS 6.6 and Ubuntu 16.04. On CentOS 6.6 I tested
with both python-virtualenv 1.7.2 (which initializes a virtualenv with
pip 1.1 and setuptools 0.6c11) and 1.10.1 (pip 1.4.1 and setuptools 0.9.8).

Change-Id: I1a8947bc8e89ec73728749bbcc6e9b919e26838a
---
M build-support/jenkins/build-and-test.sh
M python/requirements.txt
2 files changed, 24 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/7661/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7661
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1a8947bc8e89ec73728749bbcc6e9b919e26838a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] Allow tablet shutdown without completing txs

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow tablet shutdown without completing txs
..


Patch Set 11:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:

PS11, Line 270: transactions must not durably complete operations
is this precisely true? or is it that transactions are _allowed_ to abort? 
there is no CHECK ensuring that, once shutting down, nothing commits, and I 
think adding such a restriction could be tricky.

I think it is reasonable to disallow starting of any new timestamps once 
shutting down, but I dont think it's doing that yet either, right?


http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

PS11, Line 764: message
use ToString so we retain the original code?


Line 765: LOG(ERROR) << s.ToString();
add context of the tablet id, maybe the opid which should be available?

I think WARN is probably more appropriate because things are recovering without 
data loss


PS11, Line 800: exitted
typo? exited


PS11, Line 799:   if (PREDICT_FALSE(mvcc_.IsShuttingDown())) {
  : return Status::Aborted("Transaction exitted early because 
tablet is shutting down");
  :   }
mind making a short function like Tablet::CheckNotShuttingDown with this code 
in it? seems it's called from a few places


Line 852: LOG(ERROR) << "Failed to check if row is present; ending 
transaction early";
we should be able to CHECK(IsShuttingDown()) or something here, right?

I think we basically want an invariant that operations can only abort if the 
tablet is shutting down. Even if you are the one who hit the error, you need to 
ensure that it's marked shutting down before you complete the operation. 
Otherwise you could release the lock and allow some other operation to proceed 
and commit out-of-order.


Line 853: CHECK(s.IsDiskFailure());
I think this is a bit too restrictive. For example if there is some local 
heisenbug it could cause a tablet to become corrupt in some way that is not a 
disk failure error, and I think the correct thing to do in that circumstance is 
also to mark the tablet as failed (without marking all other tablets on that 
disk as failed)


http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 347:   if 
(!state()->tablet_replica()->tablet()->mvcc_manager()->IsShuttingDown()) {
I think reaching into MvccManager is a bit strange here. Even though there is a 
1:1 of tablet and mvccmanager I think it makes more sense to have the 
additional state enum in Tablet which says "shutting down". It happens that 
when the tablet shuts down, it also shuts down its MvccManager (thus allowing 
writes to abort without firing a CHECK) but conceptually the Tablet is the 
thing that is shutting down.

Also don't we already have such an enum in the TabletReplica indicating it's 
shutting down? it would be ideal if this code can avoid calling specifically 
into Tablet much.


Line 352: }
can you add FALLTHROUGH_INTENDED here?


PS11, Line 356:   VLOG_WITH_PREFIX(1) << "Transaction " << ToString() << " 
failed prior to "
  :   "replication success: " << s.ToString();
this is no longer accurate in all cases


http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS11, Line 640:   if (replica->tablet()) {
  : // If we're deleting the tablet, there's no point in 
ensuring that applies
  : // complete. Currently-running Applies will henceforth 
return early.
  : replica->tablet()->mvcc_manager()->SetShuttingDown();
  :   }
  :  
why not put this in TabletReplica::Shutdown() implementation itself so as to 
avoid spilling as many implementation details outward?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Run the gradle build as a part of the gerrit tests

[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:

(4 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.


Line 75: # 
Gerrit flagged some extra whitespace here and below.


Line 78: # Java tests.
Hmm, but we're not using gradle to run tests, just to build. Is this intended 
to be a future-proof comment?


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 
with BUILD_JAVA=0 and BUILD_GRADLE=1?


-- 
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: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [WIP] Add BlockDeletionTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: [WIP] Add BlockDeletionTransaction to Block Manager
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7656/1/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:

Line 537:   explicit FileBlockDeletionTransaction(
> warning: single-argument constructors must be marked explicit to avoid unin
Done


http://gerrit.cloudera.org:8080/#/c/7656/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1137:   explicit LogBlockDeletionTransaction(LogBlockManager* 
block_manager);
> warning: single-argument constructors must be marked explicit to avoid unin
Done


Line 1160:   BlockDataByContainerMap deleted_block_map_;
> warning: invalid case style for private member 'deleted_block_map' [readabi
Done


Line 1271: void LogBlock::RegisterDeletion(
> warning: the parameter 'transaction' is copied for each invocation but only
Done


-- 
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: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [WIP] Add BlockDeletionTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7656

to look at the new patch set (#2).

Change subject: [WIP] Add BlockDeletionTransaction to Block Manager
..

[WIP] Add BlockDeletionTransaction to Block Manager

Similar to 'BlockCreationTransaction', this patch adds a new layer
of abstraction at Block Manager to coalesce blocks deletions in a
logical operation.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tablet/tablet_metadata.cc
19 files changed, 307 insertions(+), 126 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [java] Remove double brackets from wrapper script

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java] Remove double brackets from wrapper script
..


Patch Set 2:

(1 comment)

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

PS2, Line 12: This is because the wrapper script is a shell script
: and not a bash script
FWIW, we do enforce that /bin/bash is the interpreter for all Kudu shell 
scripts. We can do that for gradlew too, if that's interesting (though if it's 
just copied into our repo verbatim from an external source, I can also 
understand keeping it this way).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I839c47bbef8bf901047b9379be958f4cebbd406e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: Reorder class declaration of LogWritableBlock

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: Reorder class declaration of LogWritableBlock
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] log block manager: Reorder class declaration of LogWritableBlock

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: log block manager: Reorder class declaration of LogWritableBlock
..


log block manager: Reorder class declaration of LogWritableBlock

Reordered class declaration of LogWritableBlock to avoid the
intermingling of LogBlockContainer and LogWritableBlock definitions
in future when adding new methods.

Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Reviewed-on: http://gerrit.cloudera.org:8080/7595
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/fs/log_block_manager.cc
1 file changed, 69 insertions(+), 65 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] log block manager: Reorder class declaration of LogWritableBlock

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7595

to look at the new patch set (#3).

Change subject: log block manager: Reorder class declaration of LogWritableBlock
..

log block manager: Reorder class declaration of LogWritableBlock

Reordered class declaration of LogWritableBlock to avoid the
intermingling of LogBlockContainer and LogWritableBlock definitions
in future when adding new methods.

Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
---
M src/kudu/fs/log_block_manager.cc
1 file changed, 69 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/7595/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] log block manager: Reorder class declaration of LogWritableBlock

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: log block manager: Reorder class declaration of LogWritableBlock
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7595/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 223: 
> This actually isn't necessary; LogBlock above already references LogBlockCo
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 15:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:

Line 282:   dirty_blocks.emplace_back(std::move(block));
> use emplace_back with r-value references.
Done


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,
> Yah I didn't realize it was being called in a closure.  Still, you could re
Thanks for pointing out!  I personally prefer to keep it as the way it is, 
because inside FinishBlock(), if the input status is not ok, we need to mark 
the container as read-only. Or maybe you have other thought?


http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1524: RETURN_NOT_OK_PREPEND(s, "unable to append block metadata");
> here and abve, start the message with a lowercase ('unable', 'container').
Done


-- 
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: 15
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] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7207

to look at the new patch set (#15).

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that for LBM container can be reused without flushing
in-flight writable blocks to disk.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 734 insertions(+), 304 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/15
-- 
To view, visit http://gerrit.cloudera.org:8080/7207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 15
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

[kudu-CR] rpc: hook up a callback for libev fatal errors

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: rpc: hook up a callback for libev fatal errors
..


Patch Set 2:

(1 comment)

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

PS2, Line 17: It's difficult to write an automated test for this, but I tested 
by
: setting my ulimit to 10 and running rpc-test.
What about using setrlimit(RLIMIT_NOFILE, ...) to drop the max number of open 
fds? Here's some test code I wrote to play around with that: 
https://gist.github.com/adembo/fba94e3956a8db4ad45d3fce91106c6b.

Alternatively, what about creating an unbounded number of Messengers in a loop? 
That would work if each one makes libev  create a pipe.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5fa77237a40f43d6bb82e9f1ceecd31d52268f9d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] disk failure: add persistent disk states

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: disk failure: add persistent disk states
..


Patch Set 9:

(10 comments)

Still working on updating this one. Note that there's now a dependency on a new 
patch to separate the dd manager from the block manager: 
https://gerrit.cloudera.org/#/c/7602/

http://gerrit.cloudera.org:8080/#/c/7270/7/src/kudu/fs/block_manager_util.h
File src/kudu/fs/block_manager_util.h:

Line 83:   //
> warning: parameter 's' is passed by value and only copied once; consider mo
Done


http://gerrit.cloudera.org:8080/#/c/7270/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

Line 212:   pool_->Shutdown();
> Done
As per a different comment, this was changed so all DataDirs, even those with 
failed instances, will have a pool.


Line 359:   initted_ = true;
> Done
Actually, I it occurred to me that it may not make sense to Create() the 
DataDirManager with failed dirs. If an operator tries to start Kudu for the 
first time with a failed disk, why be permissive?

In fact, if there is a failed disk, the disk may fail to canonicalize, and we 
wouldn't even be able to get a good mapping from UUID. I've updated this to 
just return an error for now, but let me know what you think.


http://gerrit.cloudera.org:8080/#/c/7270/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

Line 38: #include "kudu/fs/fs_manager.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


Line 387: InsertOrDie(_by_root, root, all_uuids[idx]);
> warning: passing result of std::move() as a const reference argument; no mo
Removed to not support disk failures on Create()


Line 398: if (flags & kFlagCreateTestHolePunch) {
> warning: 'd' used after it was moved [misc-use-after-move]
likewise


http://gerrit.cloudera.org:8080/#/c/7270/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

Line 204:   static const char* kDataDirName;
> warning: invalid case style for global constant 'FLAG_CREATE_TEST_HOLE_PUNC
Done


Line 205:   static const char* kInvalidPath;
> warning: invalid case style for global constant 'FLAG_CREATE_FSYNC' [readab
Done


Line 350:   FRIEND_TEST(DataDirGroupTest, TestLoadBalancingBias);
> warning: parameter 'dir_to_update' is const-qualified in the function decla
Done


Line 350:   FRIEND_TEST(DataDirGroupTest, TestLoadBalancingBias);
> warning: function 'kudu::fs::DataDirManager::WritePathHealthStates' has a d
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: 9
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] log block manager: Reorder class declaration of LogWritableBlock

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: log block manager: Reorder class declaration of LogWritableBlock
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7595/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 223: class LogBlockContainer;
This actually isn't necessary; LogBlock above already references 
LogBlockContainer by pointer and the class is forward declared in 
log_block_manager.h.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a5192cccac1931fd1f65b11fa0739090e8368e2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7602

to look at the new patch set (#6).

Change subject: separate DataDirManager from BlockManagers
..

separate DataDirManager from BlockManagers

Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.

Some logic previously in the FsManager and BlockManagers is moved into
the DataDirManager:
- canonicalization of data roots now occurs in DataDirManager. This will
  be useful, as it will enable that the DataDirManager can know all data
  directories, even those that fail to open/canonicalize
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
  instead of shutting down the DataDirManager as to not directory affect
  the DataDirManager

To clarify some vocabulary:
- Root: a top-level directory specified by 'fs_data_dirs'
- Data (root) dir: a subdirectory of a data root, named 'data'. Blocks
  are placed in this directory.

Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.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/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 459 insertions(+), 310 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/7602/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7602
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 14:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:

Line 282:   dirty_blocks.push_back(std::move(block));
use emplace_back with r-value references.


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) {
Hmm I'm not sure this is doing what we want when 
block_manager_flush_on_finalize = false.  Consider this sequence of events:

  FLAGS_block_manager_flush_on_finalize = false;
  FLAGS_block_coalesce_close = true;
  block.Finalize(); // Finalizes the block, but doesn't flush it
  block_manager.CloseBlocks( { move(block) } ); // Doesn't flush the block, 
since it's already finalized.

I think in that case CloseBlocks actually should async flush the block.


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,
> Sorry, I do not quite follow your comment. You mean FinishBlock is always c
Yah I didn't realize it was being called in a closure.  Still, you could remove 
the Status param and change line 1431 to be:

s = s.AndThen([&] { return container_->FinishBlock(this, round_mode, 
block_offset_); });


http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1524: RETURN_NOT_OK_PREPEND(s, "Unable to append block metadata");
here and abve, start the message with a lowercase ('unable', 'container').


-- 
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] KUDU-871. Support tombstoned voting

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-871. Support tombstoned voting
..


Patch Set 6:

(7 comments)

Glanced through one of the tests.  Will take a closer look today.

http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/integration-tests/tombstoned_voting-stress-test.cc
File src/kudu/integration-tests/tombstoned_voting-stress-test.cc:

PS6, Line 146: LOG(WARNING)
Why warning?  Is it something non-regular or non-expected?  Would INFO be more 
appropriate here?

Also, is it crucial to have these INFO logs output from the test?


PS6, Line 148: LOG(FATAL)
nit: would FAIL() be more idiomatic in this case (given it's a test)?


PS6, Line 159: WARNING
INFO?  But I might be missing something here.


PS6, Line 197: 2
nit: maybe cluster_->num_tablet_servers() ?


PS6, Line 228: voter_thread
What happens to the thread if the test fails in one of the ASSERT_OK() below?  
Consider joining the thread in that case as well (it might be SIGSEGV or 
something like that otherwise).


PS6, Line 232: SleepFor(MonoDelta::FromMilliseconds(500));
Could you add some comments explaining why this delay is necessary?  What would 
happen if this delay were 10 times less?


PS6, Line 240: SleepFor(MonoDelta::FromMilliseconds(500));
ditto


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes