date:20170914

Todd Lipcon has submitted this change and it was merged.

Change subject: log_block_manager: switch from google::sparse_hash_map to 
sparsepp
..


log_block_manager: switch from google::sparse_hash_map to sparsepp

sparsepp is updated for C++11 so it enables move semantics for the map
elements. Since the block map uses ref-counted values, being able to move them
is a big win. It also claims to be generally faster even aside from the
ability to support moves.

According to [1] this data structure uses ~10% more memory than
google::sparse_hash_map. However, previous measurement indicated that 1M
blocks used about 9MB of memory, so this isn't a major consumer as far
as the overall system is concerned. It seems worth a few extra MB of
memory in order to make substantial startup time improvements.

Despite slightly more memory usage, it's still significantly better than
std::unordered_map, and also shares the benefit of avoiding any large
allocations. (std::unordered_map needs a contiguous allocation for the
buckets array).

This patch alone improved startup time 7-8x on a real host with ~11M blocks:

Before:
I0907 17:23:50.748055 12507 fs_manager.cc:335] Time spent opening block 
manager: real 108.910s  user 0.000s sys 0.001s

After:
I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block 
manager: real 14.348s user 0.000s sys 0.001s

The LBM startup benchmark (1M blocks) improved less substantially but still 
noticeably:

Before:
I0907 17:16:54.899818 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.612s  user 0.035s sys 0.002s
I0907 17:16:57.498205 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.598s  user 0.039s sys 0.001s
I0907 17:17:00.100244 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.602s  user 0.042s sys 0.000s
I0907 17:17:02.686638 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.586s  user 0.042s sys 0.000s
I0907 17:17:05.284050 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.041s sys 0.001s
I0907 17:17:07.884395 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.600s  user 0.039s sys 0.001s
I0907 17:17:10.490550 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.606s  user 0.040s sys 0.001s
I0907 17:17:13.070114 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.580s  user 0.039s sys 0.000s
I0907 17:17:15.667062 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.040s sys 0.001s
I0907 17:17:18.258447 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.591s  user 0.042s sys 0.000s

After:
I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.570s  user 0.034s sys 0.001s
I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.550s  user 0.037s sys 0.001s
I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.560s  user 0.037s sys 0.001s
I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.509s  user 0.038s sys 0.001s
I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.555s  user 0.037s sys 0.001s
I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.561s  user 0.036s sys 0.001s
I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.534s  user 0.038s sys 0.000s
I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.548s  user 0.037s sys 0.001s
I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.559s  user 0.037s sys 0.001s
I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.529s  user 0.035s sys 0.001s

[1] https://github.com/greg7mdp/sparsepp/blob/master/bench.md

Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
Reviewed-on: http://gerrit.cloudera.org:8080/8007
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/fs/log_block_manager.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
5 files changed, 35 insertions(+), 2 deletions(-)

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



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

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

Todd Lipcon has submitted this change and it was merged.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..


Add a simple benchmark to create 1M blocks and reopen LBM

Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Reviewed-on: http://gerrit.cloudera.org:8080/8006
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/fs/block_id.h
M src/kudu/fs/log_block_manager-test.cc
2 files changed, 48 insertions(+), 0 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

Todd Lipcon has posted comments on this change.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..


Patch Set 2:

(1 comment)

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

PS2, Line 797: In particular,
 :   // since we write such small blocks in this test, each block 
will likely
 :   // begin on the same 4KB page as the prior one we wrote, and 
due to the
 :   // "stable page writes" feature, each block will thus end up 
waiting
 :   // on the writeback of the prior one.
> That's only true when the filesystem block size is less than 4K though, rig
Indeed that's right. I also am not sure why so many of our systems have 2K 
blocks. Perhaps it was a default in some earlier kernel and these guys have 
just been upgraded many times.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: use move semantics to fill in the block map

Adar Dembo has posted comments on this change.

Change subject: log_block_manager: use move semantics to fill in the block map
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8fe65087585e118f91320ec39f537b5f3ad6552f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

Adar Dembo has posted comments on this change.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..


Patch Set 2: Code-Review+2

(1 comment)

Based on my understanding of mkfs block size choices, I think your assertion 
about sync_file_range and stable page writes is overly strong.

But the code is fine, so feel free to ignore and merge as is.

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

PS2, Line 797: In particular,
 :   // since we write such small blocks in this test, each block 
will likely
 :   // begin on the same 4KB page as the prior one we wrote, and 
due to the
 :   // "stable page writes" feature, each block will thus end up 
waiting
 :   // on the writeback of the prior one.
That's only true when the filesystem block size is less than 4K though, right? 
I don't know why many of our test machines wound up with 2K ext4 filesystems; 
4K is generally the default on ext4 (provided the fs isn't of a trivially small 
size).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Hello Kudu Jenkins,

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

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

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

Change subject: Avoid unnecessary vector allocations for 
ReadV/WriteV-like APIs
..

Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView instead of vector.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
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/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
29 files changed, 286 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] log block manager: switch from google::sparse hash map to sparsepp

Adar Dembo has posted comments on this change.

Change subject: log_block_manager: switch from google::sparse_hash_map to 
sparsepp
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1788. Increase Raft RPC timeout to 30sec to avoid fruitless retries.

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1788. Increase Raft RPC timeout to 30sec to avoid 
fruitless retries.
..


Patch Set 1:

(2 comments)

> does this have any nasty interaction with failure detection for other 
> situations than the one described in the jira. It seems like it might 
> potentially have since the previous timeout was less than 3x heartbeats and 
> the new one isn't.

I don't think so.

Let's look at the case where the network is slow such that each batch takes 
1.1sec to transmit. In the old code, we'd have:

t leaderfollower

0.0   send 1
1.0   timeout
1.01  send 2 
1.1   send 2   received 1 
2.01  timeout
2.02  send 3 
2.21  send 3   received 2
...

i.e even though the leader was sending a new request once every 1 second due to 
the timeout, the follower receives the requests only at the rate of one every 
1.1sec because of the transmission bottleneck.

With the new code, the leader will just wait the full 1.1sec before sending the 
new request. So, the follower will still be receiving requests once every 
1.1sec, but now all of the wire bandwidth will be used for "goodput" instead of 
each request getting successively longer and longer until reaching the 
max-batch-size.

It's true that if the network is slow enough that the transmission of a single 
batch takes more than the failure time (3x heartbeat) then the follower will be 
triggering pre-elections in between each request. But I think that was already 
the case before, and if anything this should probably reduce the frequency of 
it occurring, again by reducing wasted network traffic. To avoid this I suppose 
we could have a dynamic transmission size which gets smaller on timeouts, or 
some out-of-band "liveness only" heartbeat sent via UDP, or somesuch. But all 
of those seem like incremental work that we could do on top of this fix.

http://gerrit.cloudera.org:8080/#/c/8037/1//COMMIT_MSG
Commit Message:

PS1, Line 32: causing increased latency
> That part is less intuitive to me, where's the slowdown coming from? The no
just that when you only have two replicas, you are always waiting max(latency) 
across the two. When you have three replicas, you are taking median(latency) 
across the three. So naturally the latency with two replicas is almost always 
going to be worse than the latency with three replicas, if the replicas' 
individual latencies are all drawn from the same distribution (i.e it would not 
work this way if one of the replicas is consistently slower or farther away).


http://gerrit.cloudera.org:8080/#/c/8037/1/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:

Line 51: DEFINE_int32(consensus_rpc_timeout_ms, 3,
> This doesn't appear to prevent stacking, does it? I don't see a mechanism f
Peer::SignalRequest has this code:

  // Only allow one request at a time.
  if (request_pending_) {
return;
  }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5f47dc006dc3dfb1659a224172e1905b6bf3d2a4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [consensus] micro-cleanup on VerifyRaftConfig()

Alexey Serbin has uploaded a new change for review.

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

Change subject: [consensus] micro-cleanup on VerifyRaftConfig()
..

[consensus] micro-cleanup on VerifyRaftConfig()

Removed unused RaftConfigState parameter for VerifyRaftConfig().
In addition, fixed typos in metadata.proto inline documentation.

Change-Id: Ic5b4cb6ceacde7a0c0b2f1c75dca4b7d1681ef33
---
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/master/sys_catalog.cc
6 files changed, 17 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic5b4cb6ceacde7a0c0b2f1c75dca4b7d1681ef33
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Todd Lipcon has uploaded a new patch set (#2).

Change subject: Avoid unnecessary vector allocations for 
ReadV/WriteV-like APIs
..

Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView instead of vector.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
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/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
29 files changed, 284 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon

[kudu-CR] KUDU-2144. Add metrics for Reactor load

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-2144. Add metrics for Reactor load
..


KUDU-2144. Add metrics for Reactor load

This adds two new metrics:

1) reactor_load_percent

This measures the percentage of time that a reactor spends doing active
work (i.e not blocked in epoll_wait()). As this approaches 100%, it
indicates that the server may be reactor-bound (eg due to skew, a
performance bug, or insufficient number of reactor threads)

At first glance, it might seem like this should just be a simple counter
of cycles spent in epoll_wait(), so that metrics systems would calculate
a derived cycles/second rate. However, a simple rate metric would not
well represent the fact that there are multiple reactor threads, and
it's possible (and often the case) that only one of them is highly
loaded due to skew. Additionally, there are some bursty workloads where
the load average over minute granularity is relatively low, but when
viewed on a short time scale the reactor thread approaches 100% load.

So, to capture that, this new metric is a histogram of percentages.
Values are contributed to the histogram in the periodic TimerHandler
which runs every 100ms. So, if there is any 100ms period in which the
reactor is fully loaded, it will contribute a "100%" sample to the
histogram. We can then inspect the percentiles and the raw counts to see
if there are even any short bursts where the reactors are the
bottleneck.

To test this out, I ran rpc-bench and modified the number of server
reactors. As I added more server reactors, I could see that the load
percentage (particularly the 95th percentile) went down as each reactor
was able to spend more time idle.

2) reactor_active_latency_us

This metric takes another approach to measure potential latency issues
caused by reactors by measuring the histogram of time spent invoking
watcher callbacks. If, for example, we see that callback execution
frequently takes 100ms, we can assume that a similar amount of latency
would be contributed to any inbound or outbound RPCs associated with the
same reactor.

To test this out, I simulated KUDU-1944 (OpenSSL lock contention slowing
down socket IO) by adding a usleep(10ms) call in Socket::Recv and
running rpc-bench. I could see that the new metric shot up accordingly.

Change-Id: Ic530af2836b1c31b3d754a9e0068fc5d31aa6fbb
Reviewed-on: http://gerrit.cloudera.org:8080/8064
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-bench.cc
M src/kudu/util/metrics.h
4 files changed, 133 insertions(+), 2 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic530af2836b1c31b3d754a9e0068fc5d31aa6fbb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Avoid a few allocations while reading PBC files

Hello Kudu Jenkins,

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

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

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

Change subject: Avoid a few allocations while reading PBC files
..

Avoid a few allocations while reading PBC files

This reduces extra short-lived allocations while reading PBC files
by using faststring buffers instead of heap-allocated arrays.
For short reads (<32 bytes) these are stack-allocated. Since PBC files
use length prefixes for records, this avoids a heap allocation per record.

This, combined with the prior patch to reduce temporary vector allocations,
sped up startup on a real host with 11M blocks a few more percent:

Before:
I0907 17:27:13.600186 14550 fs_manager.cc:335] Time spent opening block 
manager: real 13.401s user 0.000s sys 0.002s

After:
I0907 17:43:46.364558 21480 fs_manager.cc:335] Time spent opening block 
manager: real 12.377s user 0.000s sys 0.001s

This also sped up the LBM startup time benchmark a little bit:

Before:
I0907 17:07:40.230087  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.495s  user 0.032s sys 0.001s
I0907 17:07:41.642105  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.412s  user 0.044s sys 0.001s
I0907 17:07:43.082852  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.441s  user 0.040s sys 0.001s
I0907 17:07:44.512603  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.043s sys 0.000s
I0907 17:07:45.956848  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.444s  user 0.041s sys 0.000s
I0907 17:07:47.386735  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.430s  user 0.037s sys 0.002s
I0907 17:07:48.787317  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.401s  user 0.041s sys 0.002s
I0907 17:07:50.244407  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.457s  user 0.038s sys 0.001s
I0907 17:07:51.682209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.438s  user 0.037s sys 0.001s
I0907 17:07:53.116209  9782 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.434s  user 0.037s sys 0.000s

After:
I0907 17:40:07.984983 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.429s  user 0.032s sys 0.001s
I0907 17:40:09.359199 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.374s  user 0.037s sys 0.000s
I0907 17:40:10.767251 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.408s  user 0.038s sys 0.001s
I0907 17:40:12.160768 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.394s  user 0.037s sys 0.001s
I0907 17:40:13.539149 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.378s  user 0.035s sys 0.001s
I0907 17:40:14.932631 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.393s  user 0.037s sys 0.000s
I0907 17:40:16.316269 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.384s  user 0.037s sys 0.000s
I0907 17:40:17.714495 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.398s  user 0.038s sys 0.000s
I0907 17:40:19.077134 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.363s  user 0.036s sys 0.001s
I0907 17:40:20.487521 10204 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.410s  user 0.037s sys 0.001s

Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f
---
M src/kudu/util/pb_util.cc
1 file changed, 15 insertions(+), 28 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

Todd Lipcon has uploaded a new change for review.

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

Change subject: Avoid unnecessary vector allocations for 
ReadV/WriteV-like APIs
..

Avoid unnecessary vector allocations for ReadV/WriteV-like APIs

This reduces extra short-lived allocations for the various
scatter-gather (aka "vectored") read/write APIs.

These APIs used to all take vector, which forces the caller to
allocate on the heap. Almost all of the call sites only need a small
constant number of slices, so the heap allocation is unnecessary
overhead.

This patch imports ArrayView from Chromium/WebRTC. This is a view
into an existing array or vector (similar to how a StringPiece is a view
into a string or character array). The vectored read/write APIs are
converted to use ArrayView instead of vector.

Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/consensus/log_util.cc
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/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
A src/kudu/util/array_view.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
28 files changed, 283 insertions(+), 146 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4eab29dad2e16cc5fce724d3bdd173f3a60cb266
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon

[kudu-CR] WIP: pb util: avoid repeated stat() calls reading files

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: pb_util: avoid repeated stat() calls reading files
..

WIP: pb_util: avoid repeated stat() calls reading files

This reduces the number of fstat syscalls while loading a host with 11M blocks
from 29.3M to 147K.

(just rebased, tests still broken)

Change-Id: I27371800604bcb20bafae7946d3b3e84af094598
---
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
2 files changed, 29 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27371800604bcb20bafae7946d3b3e84af094598
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Avoid a few allocations while reading PBC files

Todd Lipcon has posted comments on this change.

Change subject: Avoid a few allocations while reading PBC files
..


Patch Set 1:

yea, WebRTC (chromium ish) has ArrayView which is pretty nice for this kinda 
thing as well. Maybe lighter-weight and more style-consistent with the rest of 
our stuff vs gsl.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Dan Burkert has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS2, Line 116: table_identifier
> Thanks everyone for the feedback.
+1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Dan Burkert has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in 
clients
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

PS3, Line 199: setWait
> I'd be more specific about what it waits for, setWaitForTableAssigned or so
I disagree.  It's just called 'wait' in the C++ API.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java:

Line 58:   Preconditions.checkNotNull(name);
This precondition is already checked in the constructor.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java:

Line 57:   Preconditions.checkNotNull(name);
Likewise.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java:

Line 78:   Preconditions.checkNotNull(name);
Likewise


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 947: Future fut = exec.submit(new Runnable() {
why not just use deferreds / the async client?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

Hello Kudu Jenkins,

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

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

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

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..

Add a simple benchmark to create 1M blocks and reopen LBM

Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
---
M src/kudu/fs/block_id.h
M src/kudu/fs/log_block_manager-test.cc
2 files changed, 48 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] log block manager: switch from google::sparse hash map to sparsepp

Hello Kudu Jenkins,

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

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

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

Change subject: log_block_manager: switch from google::sparse_hash_map to 
sparsepp
..

log_block_manager: switch from google::sparse_hash_map to sparsepp

sparsepp is updated for C++11 so it enables move semantics for the map
elements. Since the block map uses ref-counted values, being able to move them
is a big win. It also claims to be generally faster even aside from the
ability to support moves.

According to [1] this data structure uses ~10% more memory than
google::sparse_hash_map. However, previous measurement indicated that 1M
blocks used about 9MB of memory, so this isn't a major consumer as far
as the overall system is concerned. It seems worth a few extra MB of
memory in order to make substantial startup time improvements.

Despite slightly more memory usage, it's still significantly better than
std::unordered_map, and also shares the benefit of avoiding any large
allocations. (std::unordered_map needs a contiguous allocation for the
buckets array).

This patch alone improved startup time 7-8x on a real host with ~11M blocks:

Before:
I0907 17:23:50.748055 12507 fs_manager.cc:335] Time spent opening block 
manager: real 108.910s  user 0.000s sys 0.001s

After:
I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block 
manager: real 14.348s user 0.000s sys 0.001s

The LBM startup benchmark (1M blocks) improved less substantially but still 
noticeably:

Before:
I0907 17:16:54.899818 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.612s  user 0.035s sys 0.002s
I0907 17:16:57.498205 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.598s  user 0.039s sys 0.001s
I0907 17:17:00.100244 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.602s  user 0.042s sys 0.000s
I0907 17:17:02.686638 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.586s  user 0.042s sys 0.000s
I0907 17:17:05.284050 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.041s sys 0.001s
I0907 17:17:07.884395 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.600s  user 0.039s sys 0.001s
I0907 17:17:10.490550 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.606s  user 0.040s sys 0.001s
I0907 17:17:13.070114 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.580s  user 0.039s sys 0.000s
I0907 17:17:15.667062 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.597s  user 0.040s sys 0.001s
I0907 17:17:18.258447 20839 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 2.591s  user 0.042s sys 0.000s

After:
I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.570s  user 0.034s sys 0.001s
I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.550s  user 0.037s sys 0.001s
I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.560s  user 0.037s sys 0.001s
I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.509s  user 0.038s sys 0.001s
I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.555s  user 0.037s sys 0.001s
I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.561s  user 0.036s sys 0.001s
I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.534s  user 0.038s sys 0.000s
I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.548s  user 0.037s sys 0.001s
I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.559s  user 0.037s sys 0.001s
I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening 
block manager: real 1.529s  user 0.035s sys 0.001s

[1] https://github.com/greg7mdp/sparsepp/blob/master/bench.md

Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
---
M src/kudu/fs/log_block_manager.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
5 files changed, 35 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset

[kudu-CR] Add mustache template for /table

Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
..

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] Add mustache template for /table

Will Berkeley has posted comments on this change.

Change subject: Add mustache template for /table
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

Line 63:   // or an empty string if no http address is available for the 
tserver.
> nit: indent 'or' here so it looks like part of the above bullet point
Done


http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 44: col_json["name"] = col.name();
> can you remind me how escaping works with the mustache template stuff? we d
Double braces vs 3 braces is escaped vs. unescaped, respectively.
{{my_var}} -> escaped

{{{your_var}}} -> not escaped


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2135 (part 2): don't use previously failed disks

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 (#30).

Change subject: KUDU-2135 (part 2): don't use previously failed disks
..

KUDU-2135 (part 2): don't use previously failed disks

This patch updates CheckIntegrity() to ensure that disks that have
previously failed are not used and that servers will only open data
where it's previously known to be.

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.

Once this main instance is determined, disks deemed FAILED by the main
path set are marked failed in memory and not used. Additionally,
instances that are not in the same path indicated by the main path set
are similarly not used.

Unit testing is done for the new iteration of CheckIntegrity().
A test is also updated in data_dirs-test to ensure that previoulsy
failed disks will be marked failed.

Some notes:
- 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 is not used, disks
that are successfully read from but are already marked FAILED by the
main path set are not marked HEALTHY.
- 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

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/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs.proto
5 files changed, 327 insertions(+), 116 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/30
--
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: 30
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2135 (part 2): don't use previously failed disks

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 (#29).

Change subject: KUDU-2135 (part 2): don't use previously failed disks
..

KUDU-2135 (part 2): don't use previously failed disks

This patch updates CheckIntegrity() to ensure that disks that have
previously failed are not used and that servers will only open data
where it's previously known to be.

Unit testing is done for the new iteration of CheckIntegrity().
A test is also updated in data_dirs-test to ensure that previoulsy
failed disks will be marked failed.

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

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/7270/29
--
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: 29
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM

Adar Dembo has posted comments on this change.

Change subject: Add a simple benchmark to create 1M blocks and reopen LBM
..


Patch Set 1:

(1 comment)

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

Line 793:   ASSERT_OK_FAST(block->Finalize());
> You mind if I just mark this as a TODO / known deficiency of the test? This
Sure, I don't mind.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: switch from google::sparse hash map to sparsepp

Todd Lipcon has posted comments on this change.

Change subject: log_block_manager: switch from google::sparse_hash_map to 
sparsepp
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8007/1//COMMIT_MSG
Commit Message:

> Forgot to ask: we originally switched to sparse_hash_map because of it's ex
it's slightly worse (maybe 10%) than sparse_hash_map but still way better than 
std::unordered_map. Generally the block map isn't the worst consumer of memory.

The original commit said that 1M blocks took 9M with sparse_hash_map vs 24M 
with unordered_map. I'd expect this one to use maybe 11M based on the graphs at 
https://github.com/greg7mdp/sparsepp/blob/master/bench.md

The bigger win of these maps vs unordered_map is that it doesn't allocate any 
big single array for buckets. Instead it does a lot of smaller allocations. 
This helps avoid fragmentation issues, etc.

I'll add some color to the commit message.


Line 14: This improved startup time 7-8x on a real host with ~11M blocks:
> To be clear, this improvement is just from switching to sparsepp? Or with t
just this. the copy-vs-move is a big win because the copies involved refcounts 
(i.e atomic operations) and those probably inhibited a lot of memory load 
speculation, etc, with this random-access memory workload.


http://gerrit.cloudera.org:8080/#/c/8007/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS1, Line 328: SPARSEPP_PATCHLEVEL=0
 : delete_if_wrong_patchlevel $SPARSEPP_SOURCE $SPARSEPP_PATCHLEVEL
 : if [ ! -d "$SPARSEPP_SOURCE" ]; then
 :   fetch_and_expand sparsepp-${SPARSEPP_VERSION}.tar.gz
 :   pushd $SPARSEPP_SOURCE
 :   touch patchlevel-$SPARSEPP_PATCHLEVEL
 :   popd
 : fi
> If there's not one patch, you can omit the patchlevel-0 boilerplate, as wel
I thought we wanted to start putting these in so that, if in the future we add 
a patch, and then rebase back to this patchlevel 0, it will be smart enough to 
rm and rebuild.


http://gerrit.cloudera.org:8080/#/c/8007/1/thirdparty/vars.sh
File thirdparty/vars.sh:

Line 193: # (from https://github.com/toddlipcon/sparsepp)
> Why are we pulling from your fork of sparsepp and not the main repo? Have y
yea but since then they pulled my commit in, so I'll revert back to the 
upstream.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] ref counted: fix move constructors to actually move

Todd Lipcon has submitted this change and it was merged.

Change subject: ref_counted: fix move constructors to actually move
..


ref_counted: fix move constructors to actually move

Ever since the move constructors for scoped_refptr were first implemented, they
didn't properly move. That is to say, they caused an unnecessary AddRef/Release
pair.

The issue is that they were passing through an rvalue-reference parameter 
without
properly using std::move(). See [1] for an explanation of this subtlety.

When combined with some other in-flight patches, this resulted in a substantial
improvement in LBM startup time, since a hashmap containing ref-counted elements
could relocate them without unnecessary reference count increments and 
decrements.
It's likely it will also improve performance elsewhere throughout Kudu.

This patch also pulls in an extra move constructor from the Chromium code base
which they claim helps Clang generate better warnings. I didn't verify that, but
figured I'd copy it while I was looking at the code.

[1] 
https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11

Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Reviewed-on: http://gerrit.cloudera.org:8080/8002
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
---
M src/kudu/gutil/ref_counted.h
1 file changed, 12 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] ref counted: fix move constructors to actually move

Todd Lipcon has posted comments on this change.

Change subject: ref_counted: fix move constructors to actually move
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8002/2/src/kudu/gutil/ref_counted.h
File src/kudu/gutil/ref_counted.h:

PS2, Line 252:   // Move constructor. This is required in addition to the 
conversion
 :   // constructor below in order for clang to warn about 
pessimizing moves.
> After adding this, did you run clang-tidy across the codebase to see these 
seems they are emitted during normal compilation. I tried adding this code to a 
random unit test:

scoped_refptr foo() {
  scoped_refptr bar;
  return std::move(bar);
}

and with the patch it output:

../../src/kudu/util/string_case-test.cc:32:10: warning: moving a local object 
in a return statement prevents copy elision [-Wpessimizing-move]
  return std::move(bar);
 ^
../../src/kudu/util/string_case-test.cc:32:10: note: remove std::move call here
  return std::move(bar);
 ^~   ~
1 warning generated.

(before the patch it output nothing)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tests] de-flaking catalog manager tsk-itest

Alexey Serbin has posted comments on this change.

Change subject: [tests] de-flaking catalog_manager_tsk-itest
..


Patch Set 1:

> What's the verdict on this? It seems like the test is no longer as
 > flaky as it was last week. Did we fix something?

I was thinking to take a closer look at the reason behind this test starting 
being flaky since the mentioned changelist 21b0f3d5, but I haven't done that 
yet.

As for the less flaky observed for this test, nothing has been fixed in that 
regard yet, I think the observed 'more stable behavior' was due to less load 
during running the test (or it might be more powerful machines where the test 
has been run recently).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I50cee27a579cffa7232137c7039b02a1ad4ab7eb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tests] de-flaking catalog manager tsk-itest

Todd Lipcon has posted comments on this change.

Change subject: [tests] de-flaking catalog_manager_tsk-itest
..


Patch Set 1:

What's the verdict on this? It seems like the test is no longer as flaky as it 
was last week. Did we fix something?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I50cee27a579cffa7232137c7039b02a1ad4ab7eb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Update instructions for publishing changes to the live site

Todd Lipcon has posted comments on this change.

Change subject: Update instructions for publishing changes to the live site
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fc1a40d6deedf8eb1a9d8e6d13f803f6109c06b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add mustache template for /table

Todd Lipcon has posted comments on this change.

Change subject: Add mustache template for /table
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

Line 63:   // or an empty string if no http address is available for the 
tserver.
nit: indent 'or' here so it looks like part of the above bullet point


http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 44: col_json["name"] = col.name();
can you remind me how escaping works with the mustache template stuff? we do 
have to worry about it or are all substitutions always escaped when substituted 
into the template?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-501 Redirect to leader master web UI

Todd Lipcon has posted comments on this change.

Change subject: KUDU-501 Redirect to leader master web UI
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 683:   (*output)["leader_redirect"] = Substitute("$0/$1", 
leader_http_addr, path);
I'm a little nervous of endless redirect loops.

Do you think its' worth adding a parameter here like ?redir_attempt={i++} and 
in the case that you redirect more than 3 times, go to some page explaining the 
issue? Maybe this requires a lot of plumbing, though, in which case I guess we 
can go without.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Adar Dembo has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS2, Line 466:   if (mode == TableIdentificationMode::ID) {
 : req.mutable_table()->set_table_id(table_identifier);
 :   } else if (mode == TableIdentificationMode::NAME) {
 : req.mutable_table()->set_table_name(table_identifier);
 :   } else {
 : LOG(FATAL) << "Unknown table identification mode";
 :   }
> nit: consider switch/case/default
Moot; TableIdentificationMode doesn't exist anymore.


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS2, Line 82: ID
> maybe worth it to use "UUID" here?
Moot point: this is all disambiguated via master::TableIdentifierPB now.


PS2, Line 116: table_identifier
> Nit: another option might be introducing a special type TableIdentifier, wh
Thanks everyone for the feedback.

To disambiguate, I decided to go in an entirely different direction: I plumbed 
master::TableIdentifierPB through these private functions.


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 1145: CHECK(resp.has_table_id());
> why not just return some kind of bad status here indicating that probably t
OK, I'll return a bad Status instead.


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

PS2, Line 2089:   // Because the logic that waits for table creation to finish 
uses table IDs,
  :   // the rename shouldn't interfere with it.
> maybe refer the bug fix, i.e that if we're incorrectly waiting on the table
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Alexey Serbin has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS2, Line 116: table_identifier
> maybe 'table_ref' to make it less confusing? ie a 'reference' is either an 
Nit: another option might be introducing a special type TableIdentifier, which 
would have methods HasUuid(), uuid(), HasName(), name().  The 
TableIdentificationMode would be needed only for constructor or it's possible 
to drop it at all , adding SetName() and SetUuid() methods.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

2017-09-14 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in
clients
..

Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:

Line 361:* considered to be successful.
What happens if you elect to not wait? What's the expectation here? Right now
this feels like a "Kudu dev-only" API.

http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

Line 191:* Whether to wait for all tablets to be created before the table
creation is
What happens if you set this to false? Is your client going to blow up when you
try to use the KuduTable?

Also, isn't waiting here kind of an implementation detail? It's not known to
the user that there happens to be a call to create a table, which returns after
the master accepts it but still hasn't assigned the tablets. Why are we calling
this out as an API?

PS3, Line 199: setWait
I'd be more specific about what it waits for, setWaitForTableAssigned or
something?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:

PS1, Line 583: false
> did you miss this nit?
Sorry, I did.

The value is always unset. I'll clarify.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 3): remove GetTableSchema.create table done

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1807 (part 3): remove GetTableSchema.create_table_done
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5395bd73edd9cf9d18c4cb6db4f8fb39e5a41830
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Todd Lipcon has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS2, Line 116: table_identifier
> nit: use table_id like in the method below.
maybe 'table_ref' to make it less confusing? ie a 'reference' is either an ID 
or a name. Or maybe table_specifier


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 1145: CHECK(resp.has_table_id());
why not just return some kind of bad status here indicating that probably the 
client is connecting to a very old server? I think that's preferable to 
crashing, in the context of a client which might be embedded in some other app.

At the very least let's add a more useful CHECK message


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:

PS1, Line 583: false
> is it always false or always unset?
did you miss this nit?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 2]: Add util to construct sorted disjoint interval

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2055 [part 2]: Add util to construct sorted disjoint 
interval
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8041/1/src/kudu/util/sorted_disjoint_interval.h
File src/kudu/util/sorted_disjoint_interval.h:

Line 94:   IntervalVector sorted_intervals = Traits::sort(intervals);
just looking at this quickly... curious why we need a specific Traits::sort for 
sorting intervals. Given the problem statement, I expected to see something 
like the following algorithm:

// bool = false for starting points, true for end points
vector> points;
for each interval:
  CHECK(interval.start <= interval.end) (or return error)
  points.emplace(interval.start, false);
  points.emplace(interval.end, true);
sort(points)

int active = 0;
for each (point, is_end) in points:
  if !is_end:
if (active++ == 0):
  cur_start = point
  else:
if (--active == 0):
  result.emplace_back(cur_start, point)
 
CHECK_EQ(active, 0)

Wouldn't this work without having to define any interval comparator or 
Traits::Overlap, etc, so long as the 'point_type' itself is naturally 
comparable using operatorhttp://gerrit.cloudera.org:8080/8041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I61a813c047be4882f246eaf404598e7e18fcac87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 2]: Add util to construct sorted disjoint interval

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-2055 [part 2]: Add util to construct sorted disjoint 
interval
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8041/1//COMMIT_MSG
Commit Message:

PS1, Line 9: This patch adds an utility to construct a sorted disjoint interval 
set
   : given a set of intervals. The operation to construct such one is
   : O(nlgn + n) where 'n' is the number of intervals.
maybe mention a bit what this is for? easier to review for perf if we know the 
context...


http://gerrit.cloudera.org:8080/#/c/8041/1/src/kudu/util/sorted_disjoint_interval.h
File src/kudu/util/sorted_disjoint_interval.h:

PS1, Line 40: a static
immutable?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I61a813c047be4882f246eaf404598e7e18fcac87
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in 
clients
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 595:   String tableId,
you think its' worth adding @Nullable to this and @Nonnull to 'tableName' for 
better documentation and static-analysis value? If not, no worries.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java:

PS3, Line 97: isCreateTableDone
this seems misnamed -- shouldn't it be 'waitForCreateTableCompletion' or 
something, considering it ostensibly blocks for up to the provided timeout?


Line 99: while (timeSleptMillis < getDefaultAdminOperationTimeoutMs()) {
do you think it's possible to factor out this "retry running an async method up 
to a provided timeout" pattern that we see here and in isAlterTableDone? Seems 
like the methods are the same modulo which function they call into, so maybe 
could be something like:

  private  callAsyncWithSleepsAndRetries(Callable func, String 
taskNameForLogs) { ... }


PS3, Line 166: isAlterTableDone
same, although I guess this one was already implemented, so it's hard to rename 
it without a deprecation period.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 3): remove GetTableSchema.create table done

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1807 (part 3): remove GetTableSchema.create_table_done
..


Patch Set 2:

actually nm.
my concern was that we might use done() (which is false by default) without 
has_done() but I realize that the behavior is the same if the field is absent 
from the pb

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5395bd73edd9cf9d18c4cb6db4f8fb39e5a41830
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1807 (part 3): remove GetTableSchema.create table done

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1807 (part 3): remove GetTableSchema.create_table_done
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5395bd73edd9cf9d18c4cb6db4f8fb39e5a41830
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1807 (part 3): remove GetTableSchema.create table done

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1807 (part 3): remove GetTableSchema.create_table_done
..


Patch Set 2:

lgtm, just want to make sure that previously the java client (and elsewhere) 
this was indeed being used as an optional

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5395bd73edd9cf9d18c4cb6db4f8fb39e5a41830
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2144. Add metrics for Reactor load

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-2144. Add metrics for Reactor load
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic530af2836b1c31b3d754a9e0068fc5d31aa6fbb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

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

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 19:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6968/19/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:

PS19, Line 210:   // Returns 0 if the log is shut down.
hrm, this might be misleading, as the WAL might still be taking space (e.g. for 
failed tablets that were not immediately deleted). what do other components do 
here? maybe we should return an obviously special value (like -1)?


http://gerrit.cloudera.org:8080/#/c/6968/19/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:

PS19, Line 364: OnDiskDataSize
maybe OnDiskBaseDataSize()?


PS19, Line 348:   // A rowset's space-occupying components are as follows:
  :   // - cfile set
  :   //   - base data
  :   //   - bloom file
  :   //   - ad hoc index
  :   // - delta files
  :   //   - UNDO deltas
  :   //   - REDO deltas
  : 
  :   // Return the size on-disk of this rowset, in bytes.
  :   uint64_t OnDiskSize() const OVERRIDE;
  : 
  :   // Return the on-disk size of this rowset's cfile set, in 
bytes.
  :   uint64_t CFileSetOnDiskSize() const;
  : 
  :   // Return the size on-disk of the base data (no deltas) in 
this rowset's cfile set, in bytes.
  :   uint64_t OnDiskDataSize() const OVERRIDE;
  : 
  :   // 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's cfile set's base 
data and this rowset's REDO deltas.
  :   uint64_t OnDiskDataSizeNoUndos() const OVERRIDE;
  : 
  :   size_t DeltaMemStoreSize() const OVERRIDE;
this is getting kind of messy. would it be too much work to clean this up and 
return something like:
struct DiskRowSetSpace {
   int64_t memstore_size;
   int64_t cfile_set_size;
   int64_t total_delta_size:
   int64_t redo_deltas_size;
   int64_t undo_deltas_size;
}

?


-- 
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: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
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-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] kudu-spark: change default master address to local fqdn

Todd Lipcon has posted comments on this change.

Change subject: kudu-spark: change default master address to local fqdn
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8072/2/java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
File 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala:

PS2, Line 38: InetAddress.getLocalHost.getCanonicalHostName
InetAddress.getLocalHost can throw. What happens if we throw in this context? 
Will the error message be interpretable?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Will Berkeley has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS17, Line 909: std::lock_guard
> Andrew is right. You can use shared_lock with a percpu_rwlock, in the way t
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

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

Hello Adar Dembo, Alexey Serbin, 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 (#19).

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/integration-tests/tablet_copy-itest.cc
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
24 files changed, 279 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/6968/19
-- 
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: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

2017-09-14 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS2, Line 466:   if (mode == TableIdentificationMode::ID) {
 : req.mutable_table()->set_table_id(table_identifier);
 :   } else if (mode == TableIdentificationMode::NAME) {
 : req.mutable_table()->set_table_name(table_identifier);
 :   } else {
 : LOG(FATAL) << "Unknown table identification mode";
 :   }
nit: consider switch/case/default


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS2, Line 82: ID
maybe worth it to use "UUID" here?


PS2, Line 116:  const std::string& table_identifier,
actually now I understand the difference, not obvious at the first read though. 
maybe call this "table_id" and then use "table_uuid" or "table_name" or 
somesuch?


PS2, Line 116: table_identifier
nit: use table_id like in the method below.


http://gerrit.cloudera.org:8080/#/c/8066/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

PS2, Line 2089:   // Because the logic that waits for table creation to finish 
uses table IDs,
  :   // the rename shouldn't interfere with it.
maybe refer the bug fix, i.e that if we're incorrectly waiting on the table 
name then this test will fail


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] kudu-spark: change default master address to local fqdn

2017-09-14 Thread Attila Bukor (Code Review)

Attila Bukor has posted comments on this change.

Change subject: kudu-spark: change default master address to local fqdn
..


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8072/2/java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
File 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala:

Line 24: import org.apache.spark.sql.SparkSession
> what happened to SQLContext? was it unused?
never mind, checked it in the meantime, it was unused


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Adar Dembo has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS17, Line 909: std::lock_guard
> I think percpu_rwlock doesn't support shared_lock.
Andrew is right. You can use shared_lock with a percpu_rwlock, in the way that 
he pointed out:

  shared_lock l(state_lock_.get_lock());

See L783 in log.cc for an example.


-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
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-spark: change default master address to local fqdn

2017-09-14 Thread Attila Bukor (Code Review)

Attila Bukor has posted comments on this change.

Change subject: kudu-spark: change default master address to local fqdn
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8072/2/java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
File 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala:

Line 24: import org.apache.spark.sql.SparkSession
what happened to SQLContext? was it unused?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Hello Adar Dembo, Alexey Serbin, 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 (#18).

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/integration-tests/tablet_copy-itest.cc
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
24 files changed, 279 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/6968/18
-- 
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: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

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

Will Berkeley has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS17, Line 909: std::lock_guard
> Does this need to be lock_guard? Could it be lock_shared l(sta
I think percpu_rwlock doesn't support shared_lock.


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

PS17, Line 736: OnDiskDataSize()
> nit: may be worth commenting in the API declaration that this does not incl
Added (no deltas) for clarity. The explanatory comment above the declarations, 
when matched with the comments on the declarations, does make it clear exactly 
what a given *OnDiskSize* metric includes.


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_metadata-test.cc
File src/kudu/tablet/tablet_metadata-test.cc:

PS17, Line 138: The on-disk size 
> nit: maybe "The tablet metadata"?
Done


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:

PS17, Line 193: ) != nullptr
> nit: at L701, you have a similar check that doesn't make this explicit comp
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
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] [iwyu] introduce Linux system mappings

Alexey Serbin has submitted this change and it was merged.

Change subject: [iwyu] introduce Linux system mappings
..


[iwyu] introduce Linux system mappings

For starters, added the following mappings to achieve better
compatibility across different Linus versions and distros:

  * use  even if  is available
  * use  even if  is available
  * use  as an umbrella header file for various asm types

Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Reviewed-on: http://gerrit.cloudera.org:8080/8071
Reviewed-by: Adar Dembo 
Reviewed-by: Andrew Wong 
Tested-by: Kudu Jenkins
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/env_posix.cc
4 files changed, 48 insertions(+), 13 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, but someone else must approve
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2125: Tablet copy client does not retry on failures

Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-2125: Tablet copy client does not retry on failures
..


KUDU-2125: Tablet copy client does not retry on failures

The tablet copy client would fail the tablet copy after encountering an
RPC error. This commit adds retry logic for tablet copy operations when
encountering SERVER_TOO_BUSY or UNAVAILABLE errors, which are retriable.

Change-Id: I7c8454fc600a841bd15306a2b3b06ddf53130be6
Reviewed-on: http://gerrit.cloudera.org:8080/8016
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy_client_session-itest.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
5 files changed, 139 insertions(+), 33 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7c8454fc600a841bd15306a2b3b06ddf53130be6
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession

Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-2124. Don't hold session lock while initializing a 
TabletCopySession
..


KUDU-2124. Don't hold session lock while initializing a TabletCopySession

This patch replaces the interior mutex in TabletCopySourceSession with a
dynamic once, so that initialization can happen concurrently. This
allows initialization to happen outside of the critical section in the
tablet copy service.

Implemented a regression test that injects latency into
BeginTabletCopySession() calls.

Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Reviewed-on: http://gerrit.cloudera.org:8080/7985
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/tserver/tablet_server_test_util.h
11 files changed, 243 insertions(+), 62 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] kudu-spark: change default master address to local fqdn

Hello Kudu Jenkins,

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

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

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

Change subject: kudu-spark: change default master address to local fqdn
..

kudu-spark: change default master address to local fqdn

The previous default was 'localhost', which isn't resolvable in a
cluster context. It would also cause issues on Kerberized clusters due
to 'localhost' not matching the Kerberos principal name of the master
(see KUDU-2142).

Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
---
M 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
M 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
3 files changed, 23 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [iwyu] introduce Linux system mappings

Andrew Wong has posted comments on this change.

Change subject: [iwyu] introduce Linux system mappings
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-501 Redirect to leader master web UI

Andrew Wong has posted comments on this change.

Change subject: KUDU-501 Redirect to leader master web UI
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

PS2, Line 650:   if (!s.ok()) {
 : s = s.CloneAndPrepend("unable to list masters");
 : return s;
 :   }
RETURN_NOT_OK_PREPEND?


http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.h
File src/kudu/master/master-path-handlers.h:

PS2, Line 79: output
nit: can you surround this in 's? At first glance it didn't read as a variable 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Alexey Serbin has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 2: Code-Review+1

(1 comment)

LGTM, but perhaps Todd or JD need to approve the introduced 'incompatibility' 
with pre-0.10 servers.

http://gerrit.cloudera.org:8080/#/c/8066/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS1, Line 1145: CHECK(resp.has_table_id());
> We could, but the version of Kudu that doesn't return a table ID in AlterTa
SGTM


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in 
clients
..


Patch Set 3: Verified+1

Overriding Jenkins, the sole failure was in setting up one of the TSAN tests:

  INFO142   isolateserver(1843): Retrieving remaining files (65 of them)...
  ERROR  18226   isolateserver(599): Failed to fetch 
87cf40e5c6969e2c3a07236da9d950dd693ea974: [Errno 104] Connection reset by peer

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Hello Kudu Jenkins,

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

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

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

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..

c++ client: try harder to pass table IDs into RPCs that can accept them

Some RPCs (such as IsCreateTableDone or IsAlterTableDone) can identify a
table by name or by ID. Using an ID is more robust since it's globally
unique and immutable. This patch changes several RPC calls to use table IDs.

Note: table IDs were only added to AlterTableResponsePB in Kudu 0.10. By
using them here, the C++ client may crash when altering tables belonging to
an older deployment.

Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
5 files changed, 116 insertions(+), 52 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] c++ client: try harder to pass table IDs into RPCs that can accept them

Adar Dembo has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8066/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS1, Line 1145: CHECK(resp.has_table_id());
> Would it make sense to use the name of the table passed into the constructo
We could, but the version of Kudu that doesn't return a table ID in 
AlterTableResponsePB is so old (0.9) that I don't think it merits even several 
lines of fallback behavior.


http://gerrit.cloudera.org:8080/#/c/8066/1/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:

PS1, Line 205:   MonoTime now;
 :   do {
 : bool create_in_progress;
 : ASSERT_OK(client_->IsCreateTableInProgress(kTableName, 
_in_progress));
 : if (!create_in_progress) {
 :   break;
 : }
 : SleepFor(MonoDelta::FromMilliseconds(100));
 : now = MonoTime::Now();
 :   } while (now < deadline);
 :   ASSERT_TRUE(now < deadline) << "Timed out waiting for 
CreateTable to finish";
> Could ASSERT_EVENTUALLY() fit here?
Good idea.

I used it below as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] introduce Linux system mappings

Adar Dembo has posted comments on this change.

Change subject: [iwyu] introduce Linux system mappings
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] [iwyu] introduce Linux system mappings

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] introduce Linux system mappings
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8071/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 73: #include 
> warning: #includes are not sorted properly [llvm-include-order]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-501 Redirect to leader master web UI

Adar Dembo has posted comments on this change.

Change subject: KUDU-501 Redirect to leader master web UI
..


Patch Set 2:

(5 comments)

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

PS2, Line 12: a
Nit: an


http://gerrit.cloudera.org:8080/#/c/8068/2/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 214: (*output)["error"] = Substitute("Master is not ready: ", 
l.catalog_status().ToString());
Missing an $0 here.

A unit test would have caught this. Perhaps you could add one? We have the 
PeriodicWebUIChecker and several multi-master tests you could reuse.


PS2, Line 654:  ServerEntryPB
Nit: auto is fine here too, since the scope is short.


Line 671:   return Status::NotFound("no leader master known to this master");
Do you think it would it be useful to add some representation of 'masters' to 
the message? Perhaps the errors, if they exist?


http://gerrit.cloudera.org:8080/#/c/8068/2/www/table.mustache
File www/table.mustache:

Line 26:   You can find this page on the leader master's 
web UI.
Hmm, we can't do a fancy HTTP automatic redirect? Via a 3xx status code 
(https://en.wikipedia.org/wiki/URL_redirection#HTTP_status_codes_3xx)?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] introduce Linux system mappings

Hello Kudu Jenkins,

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

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

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

Change subject: [iwyu] introduce Linux system mappings
..

[iwyu] introduce Linux system mappings

For starters, added the following mappings to achieve better
compatibility across different Linus versions and distros:

  * use  even if  is available
  * use  even if  is available
  * use  as an umbrella header file for various asm types

Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/env_posix.cc
4 files changed, 48 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

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

Andrew Wong has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS17, Line 909: std::lock_guard
Does this need to be lock_guard? Could it be lock_shared 
l(state_lock_.get_lock()) instead?


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

PS17, Line 736: OnDiskDataSize()
nit: may be worth commenting in the API declaration that this does not include 
undos/redos? I think it only says "no metadata" right now. 

Intuitively, I would expect OnDiskDataSize() to be larger than 
OnDiskDataSizeNoUndos(). Maybe OnDiskDataSizeWithRedos or something?


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_metadata-test.cc
File src/kudu/tablet/tablet_metadata-test.cc:

PS17, Line 138: The on-disk size 
nit: maybe "The tablet metadata"?


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:

PS17, Line 193: ) != nullptr
nit: at L701, you have a similar check that doesn't make this explicit 
comparison to nullptr


-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
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] c++ client: try harder to pass table IDs into RPCs that can accept them

Alexey Serbin has posted comments on this change.

Change subject: c++ client: try harder to pass table IDs into RPCs that can 
accept them
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8066/1/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS1, Line 1145: CHECK(resp.has_table_id());
Would it make sense to use the name of the table passed into the constructor of 
the object in that case (KuduTableAlterer::KuduTableAlterer(KuduClient* client, 
const string& name)) instead of crashing?


http://gerrit.cloudera.org:8080/#/c/8066/1/src/kudu/integration-tests/master_failover-itest.cc
File src/kudu/integration-tests/master_failover-itest.cc:

PS1, Line 205:   MonoTime now;
 :   do {
 : bool create_in_progress;
 : ASSERT_OK(client_->IsCreateTableInProgress(kTableName, 
_in_progress));
 : if (!create_in_progress) {
 :   break;
 : }
 : SleepFor(MonoDelta::FromMilliseconds(100));
 : now = MonoTime::Now();
 :   } while (now < deadline);
 :   ASSERT_TRUE(now < deadline) << "Timed out waiting for 
CreateTable to finish";
Could ASSERT_EVENTUALLY() fit here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0052d18f714aee1226b96bf1da9defa5738fdd37
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] kudu-spark-tools: change default master address to local fqdn

Hello Attila Bukor, Todd Lipcon,

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

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

to review the following change.

Change subject: kudu-spark-tools: change default master address to local fqdn
..

kudu-spark-tools: change default master address to local fqdn

The previous default was 'localhost', which isn't resolvable in a
cluster context. It would also cause issues on Kerberized clusters  due
to 'localhost' not matching the Kerberos principal name of the master
(see KUDU-2142).

Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
---
M 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
1 file changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I10ec7414c451f54b95d86663d743162688e304ba
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Todd Lipcon

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

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17: Code-Review+1

-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] [iwyu] introduce Linux system mappings

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] introduce Linux system mappings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8071/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 79: #include   // IWYU pragma: keep
> Still need this pragma?
Good catch!  Nope, this is no longer needed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] introduce Linux system mappings

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [iwyu] introduce Linux system mappings
..

[iwyu] introduce Linux system mappings

For starters, added the following mappings to achieve better
compatibility across different Linus versions and distros:

  * use  even if  is available
  * use  even if  is available
  * use  as an umbrella header file for various asm types

Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/env_posix.cc
4 files changed, 48 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [iwyu] introduce Linux system mappings

Adar Dembo has posted comments on this change.

Change subject: [iwyu] introduce Linux system mappings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8071/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 79: #include   // IWYU pragma: keep
Still need this pragma?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] introduce Linux system mappings

Alexey Serbin has uploaded a new change for review.

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

Change subject: [iwyu] introduce Linux system mappings
..

[iwyu] introduce Linux system mappings

For starters, added the following mappings to achieve better
compatibility across different Linus versions and distros:

  * use  even if  is available
  * use  even if  is available
  * use  as an umbrella header file for various asm types

Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/env_posix.cc
4 files changed, 47 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4e50f9135fa076e429b26e3919ef19eb5d430173
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin

[kudu-CR] KUDU-2125: Tablet copy client does not retry on failures

Dan Burkert has posted comments on this change.

Change subject: KUDU-2125: Tablet copy client does not retry on failures
..


Patch Set 15: Code-Review+2

Carrying over Mike's +2.  Had to rebase due to test conflict.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c8454fc600a841bd15306a2b3b06ddf53130be6
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession

Dan Burkert has posted comments on this change.

Change subject: KUDU-2124. Don't hold session lock while initializing a 
TabletCopySession
..


Patch Set 10: Code-Review+2

(1 comment)

Carrying over Mike's +2.  Had to rebase due to merge conflict in test.

http://gerrit.cloudera.org:8080/#/c/7985/8/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:

Line 1019: if (error_code) *error_code = error.code();
> I'm not sure if you did this or I did this, but if error_code is passed in 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession

Hello Mike Percy, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2124. Don't hold session lock while initializing a 
TabletCopySession
..

KUDU-2124. Don't hold session lock while initializing a TabletCopySession

This patch replaces the interior mutex in TabletCopySourceSession with a
dynamic once, so that initialization can happen concurrently. This
allows initialization to happen outside of the critical section in the
tablet copy service.

Implemented a regression test that injects latency into
BeginTabletCopySession() calls.

Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/tserver/tablet_server_test_util.h
11 files changed, 243 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/7985/10
-- 
To view, visit http://gerrit.cloudera.org:8080/7985
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-2125: Tablet copy client does not retry on failures

2017-09-14 Thread Mike Percy (Code Review)

Mike Percy has posted comments on this change.

Change subject: KUDU-2125: Tablet copy client does not retry on failures
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c8454fc600a841bd15306a2b3b06ddf53130be6
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

Will Berkeley has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
..


Patch Set 17:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6968/16/src/kudu/consensus/log-test.cc
File src/kudu/consensus/log-test.cc:

PS16, Line 1166: ASSERT_EQ(3, anchors.size());
> nit: the expected value comes first
Done


http://gerrit.cloudera.org:8080/#/c/6968/15/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS15, Line 910: f the log is closed, the
> What happens with the log when it closes?  Does it effective size go zero i
I believe the latter is the case-- the log is closed when the tablet is 
shutdown, which I believe happens only when the tablet is deleted or 
tombstoned. I added a comment explaining this.


http://gerrit.cloudera.org:8080/#/c/6968/16/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:

PS16, Line 24: #include 
> nit: prefer  for C++11 (I need to update IWYU stuff to give proper
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

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

Hello Adar Dembo, 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 (#17).

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/integration-tests/tablet_copy-itest.cc
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
24 files changed, 279 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/6968/17
-- 
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: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] KUDU-501 Redirect to leader master web UI

Will Berkeley has posted comments on this change.

Change subject: KUDU-501 Redirect to leader master web UI
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8068/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 142: void MasterPathHandlers::HandleCatalogManager(const 
Webserver::WebRequest& req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-501 Redirect to leader master web UI

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-501 Redirect to leader master web UI
..

KUDU-501 Redirect to leader master web UI

The /table and /tables page of the master web UI only work
on the leader master. Previously, we just showed a gross error when the
user tried to access these pages on a non-leader. This adds a nice
redirect instead (or a error about why a redirect wasn't possible).

Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M www/table.mustache
M www/tables.mustache
4 files changed, 74 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add mustache template for /table

Hello Kudu Jenkins,

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

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

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

Change subject: Add mustache template for /table
..

Add mustache template for /table

As a run-up to improving the web ui experience when there are multiple
masters, this patch switches the /table page to be based on a mustache
template.

Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/server/webui_util.cc
M src/kudu/server/webui_util.h
A www/table.mustache
M www/tables.mustache
6 files changed, 314 insertions(+), 182 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley

[kudu-CR] Add mustache template for /table

Will Berkeley has posted comments on this change.

Change subject: Add mustache template for /table
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8067/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 336: state_json["percentage"] = tablets.size() == 0 ? "0.0" : 
StringPrintf("%.2f", percentage);
> warning: the 'empty' method should be used to check for emptiness instead o
Done


Line 347: void MasterPathHandlers::HandleMasters(const Webserver::WebRequest& 
req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


Line 632:   } else {
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe78e0701c1cd965650b101244212b5988f11fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2144. Add metrics for Reactor load

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2144. Add metrics for Reactor load
..

KUDU-2144. Add metrics for Reactor load

This adds two new metrics:

1) reactor_load_percent

This measures the percentage of time that a reactor spends doing active
work (i.e not blocked in epoll_wait()). As this approaches 100%, it
indicates that the server may be reactor-bound (eg due to skew, a
performance bug, or insufficient number of reactor threads)

At first glance, it might seem like this should just be a simple counter
of cycles spent in epoll_wait(), so that metrics systems would calculate
a derived cycles/second rate. However, a simple rate metric would not
well represent the fact that there are multiple reactor threads, and
it's possible (and often the case) that only one of them is highly
loaded due to skew. Additionally, there are some bursty workloads where
the load average over minute granularity is relatively low, but when
viewed on a short time scale the reactor thread approaches 100% load.

So, to capture that, this new metric is a histogram of percentages.
Values are contributed to the histogram in the periodic TimerHandler
which runs every 100ms. So, if there is any 100ms period in which the
reactor is fully loaded, it will contribute a "100%" sample to the
histogram. We can then inspect the percentiles and the raw counts to see
if there are even any short bursts where the reactors are the
bottleneck.

To test this out, I ran rpc-bench and modified the number of server
reactors. As I added more server reactors, I could see that the load
percentage (particularly the 95th percentile) went down as each reactor
was able to spend more time idle.

2) reactor_active_latency_us

This metric takes another approach to measure potential latency issues
caused by reactors by measuring the histogram of time spent invoking
watcher callbacks. If, for example, we see that callback execution
frequently takes 100ms, we can assume that a similar amount of latency
would be contributed to any inbound or outbound RPCs associated with the
same reactor.

To test this out, I simulated KUDU-1944 (OpenSSL lock contention slowing
down socket IO) by adding a usleep(10ms) call in Socket::Recv and
running rpc-bench. I could see that the new metric shot up accordingly.

Change-Id: Ic530af2836b1c31b3d754a9e0068fc5d31aa6fbb
---
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-bench.cc
M src/kudu/util/metrics.h
4 files changed, 132 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic530af2836b1c31b3d754a9e0068fc5d31aa6fbb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mike Percy

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

2017-09-14 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in 
clients
..


Patch Set 2:

(1 comment)

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

PS2, Line 20: with TABLET_NOT_RUNNING
:   master errors
> When the master responds to GetTableLocations with a TABLET_NOT_RUNNING err
Oh really, that's unexpected. I was afraid you'd point me at 
https://github.com/apache/kudu/blob/master/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java#L313
 instead. It's weird that it's sending a ServiceUnavailable though, like the 
master itself is fine.


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

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

[kudu-CR] KUDU-501 Redirect to leader master web UI

Will Berkeley has uploaded a new change for review.

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

Change subject: KUDU-501 Redirect to leader master web UI
..

KUDU-501 Redirect to leader master web UI

The /table and /tables page of the master web UI only work
on the leader master. Previously, we just showed a gross error when the
user tried to access these pages on a non-leader. This adds a nice
redirect instead (or a error about why a redirect wasn't possible).

Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M www/table.mustache
M www/tables.mustache
4 files changed, 72 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If31543b7899976ec02704111e9e789035c44dfe1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley

[kudu-CR] Add mustache template for /table