date:20170417

Adar Dembo has posted comments on this change.

Change subject: KUDU-1952 Improve block placement
..


Patch Set 1:

(5 comments)

I didn't do a full review, but left you a couple comments to get started.

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

Line 402:   if (group_by_tablet_map_.find(tablet_id) == 
group_by_tablet_map_.end()) {
Take a look at map-util.h; it's got a lot of idiomatic methods for dealing with 
maps. For example, one of the LookupOrInsert() variants might be more 
appropriate here.


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

Line 47: typedef std::vector DataDirGroup;
I think it makes sense to define this as a class with a corresponding PB class 
(in fs.proto). Then, you can encapsulate  ToPB() and FromPB() methods here. 
After that, maybe you can rework the DataDirGroup functions below such that 
it's clear which ones take DataDirGroupPB (because they're loading it from 
serialized state) and which ones take DataDirGroup?


http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/delta_compaction.h
File src/kudu/tablet/delta_compaction.h:

Line 67:   Status Compact(const fs::CreateBlockOptions& opts);
A compaction is only ever for a single tablet; can we just get the tablet ID 
when we construct the MajorDeltaCompaction and avoid this plumbing?


http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/multi_column_writer.h
File src/kudu/tablet/multi_column_writer.h:

Line 40: struct CreateBlockOptions;
In general it feels weird to use CreateBlockOptions as the vehicle for plumbing 
the tablet's ID down into the WritableBlock. Instead, let's call a spade a 
spade and just pass the tablet ID explicitly instead. Reserve 
CreateBlockOptions for interacting with the FsManager (or block manager).


http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

Line 335:   fs::DataDirGroup* data_dir_group_;
It would simplify a lot if we could avoid this link between the tablet metadata 
and the group. Can we fetch it as needed from the dd_manager?

Even better, when we need to serialize it, can we offer a mutable pointer of 
the superblock's DataDirGroupPB to the dd_manager, and have it deal with 
serialization and writing?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1970: node density integration test

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-1970: node density integration test
..


Patch Set 1:

(1 comment)

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

Line 20: WIP because an itest isn't a great fit; we're not actually testing 
anything.
isn't it a sort of stress test, in that we're testing larger data volumes than 
other tests do?

I've been pondering for years now the idea of changing KUDU_ALLOW_SLOW_TESTS=1 
to KUDU_TEST_LENGTH={fast,slow,veryslow}, and this could be the first of the 
third category, which we might not run pre-commit but could run nightly or 
post-commit.

It could also be named '-bench' as in rpc-bench, and added to the benchmarks 
script, so we could plot startup time performance, thread count, memory usage, 
etc, of the test like we plot other numeric metrics from build to build.


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

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

[kudu-CR] WIP: KUDU-1970: node density integration test

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: WIP: KUDU-1970: node density integration test
..

WIP: KUDU-1970: node density integration test

This patch introduces a new itest that models a storage-dense Kudu
deployment. The idea is simple: rather than actually generating and storing
lots of data (which is both time intensive and developer unfriendly), let's
produce a lot of metadata instead, as that's cheaper and can proxy for data.
The test itself isn't that interesting; most of the challenge was in running
it repeatedly to determine which flag values yielded the most metadata.

In a run of the test on a 48 core el6.6 machine (max_blocks_per_container=8
and num_seconds=240), I produced ~110K blocks across ~21k LBM containers,
which yielded a subsequent ~100s LBM startup time.

WIP because an itest isn't a great fit; we're not actually testing anything.
But, it didn't really make sense as part of the CLI tool either as that
talks to existing clusters/deployments and never spins up an
ExternalMiniCluster (and I'm not convinced it could either; there are some
Kudu testing assumptions baked into EMC). I'm open to other ideas.

Change-Id: Ie9b5d01557eb41d386ce92f576ed01ec658e8e7d
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/dense_node-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/test_workload.h
4 files changed, 261 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9b5d01557eb41d386ce92f576ed01ec658e8e7d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere

Todd Lipcon has posted comments on this change.

Change subject: WIP: simplify MemTracker and move process throttling elsewhere
..


Patch Set 2:

Sounds good, I'll try to push this to commitable status in the next day or two 
including running a stress workload to make sure the actual memory usage stays 
within bounds, perf didn't regress, etc.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] gutil: stop supporting AMD Opteron K8

Adar Dembo has posted comments on this change.

Change subject: gutil: stop supporting AMD Opteron K8
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere

Adar Dembo has posted comments on this change.

Change subject: WIP: simplify MemTracker and move process throttling elsewhere
..


Patch Set 2:

> do you have any reservations about this particular patch?
 > Or just suggesting it might be easier to start clean?
 
No reservations, just thought it'd be net simpler to start clean.

 > If you aren't strongly against, I'd like to continue down this
 > route.

Sure, works for me. I'll review this in depth once you're comfortable that it's 
no longer WIP.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] gutil: stop supporting AMD Opteron K8

Hello Kudu Jenkins,

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

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

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

Change subject: gutil: stop supporting AMD Opteron K8
..

gutil: stop supporting AMD Opteron K8

This removes a workaround for a bug in very old (circa 2003) Opteron
chips. The workaround was producing an extra branch around most of our
atomic operations. This likely has a negligible perf impact, but it's
possible the slightly smaller code would make us run a bit faster or
reduce branch mispredictions.

This also adds a missing call to init the feature flags in the first
place, which we seem to have forgotten the first time around.

Impala made a similar change[1][2] a while back and seems to have had
no real issues.

[1] https://gerrit.cloudera.org/#/c/2516/
[2] 
https://github.com/apache/incubator-impala/commit/a7c3f301bb1da14d6bb35d37aab47450c67cca0c

Change-Id: I59b5aeaae56b5f310b5dd160b868fcb5d04f4da1
---
M src/kudu/gutil/atomicops-internals-tsan.h
M src/kudu/gutil/atomicops-internals-x86.cc
M src/kudu/gutil/atomicops-internals-x86.h
M src/kudu/util/init.cc
4 files changed, 11 insertions(+), 35 deletions(-)


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

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

[kudu-CR] Allow forcing color clang diagnostics with an env var

Todd Lipcon has submitted this change and it was merged.

Change subject: Allow forcing color clang diagnostics with an env var
..


Allow forcing color clang diagnostics with an env var

Change-Id: I3161d6dcc3715693771089351aadb29810c00fcc
Reviewed-on: http://gerrit.cloudera.org:8080/6619
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)

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



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

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

[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere

Todd Lipcon has posted comments on this change.

Change subject: WIP: simplify MemTracker and move process throttling elsewhere
..


Patch Set 2:

Adar -- do you have any reservations about this particular patch? Or just 
suggesting it might be easier to start clean?

If you aren't strongly against, I'd like to continue down this route. Once this 
patch is committed, we'll have better separation between MemTracker and 
throttling logic, and can make some progress on improving the throttling vs 
flush-triggering issues discussed in 
https://docs.google.com/document/d/17-2CcmrjxZY0Gd9wDUh83xCCNL574famw8_2Bhfu-_8/edit

 The missing piece that makes this a WIP is periodically triggering the 
tcmalloc ReleaseMemory() call as necessary, after which point I think it should 
be committable.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: use extent maps to decide whether to truncate containers

Todd Lipcon has posted comments on this change.

Change subject: log block manager: use extent maps to decide whether to 
truncate containers
..


Patch Set 6:

Yea, I think it's quite reasonable to only do so in a "fs check --repair" type 
situation.

Another option I think is reasonable is to use stat() on the container file, 
see the actual space usage, and if it's significantly more than we expect based 
on the live block count in the container, trigger the hole-punching.

A third option: when we successfully hole-punch after a delete, write an entry 
to the metadata file saying "hole-punched!" (no need to fsync it). On restart, 
re-punch any containers which have deletions at the trail of the metadata with 
no "hole-punched!". This way you'd only have to re-punch those files which 
might have missed their chance due to a premature process exit last time around.

BTW the restart times are longer than i expected, would be curious to know 
where all that time is going (but that's unrelated to this jira)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: use extent maps to decide whether to truncate containers

Adar Dembo has posted comments on this change.

Change subject: log block manager: use extent maps to decide whether to 
truncate containers
..


Patch Set 6:

> Have you done any tests of how this affects startup time,
 > particularly after a full drop_caches on a server with a large
 > number of containers/extents? Am worried that we might regress
 > startup time significantly

I've been working on an itest that, amongst other things, produces an enormous 
number of blocks and LBM containers, many of them full. The itest can also 
benchmark LBM startup time after first calling sync() and writing '3' to 
/proc/sys/vm/drop_caches. I tested it on my Ubuntu 16.04 laptop (ssd) as well 
as an el6.6 physical machine (hdd), with this patch series and without.

Here are the full results from my first attempt:  
https://gist.github.com/adembo/0caccb85541789572e02e9e683341359. I was feeling 
OKish about the (not too significant) change, but then I noticed that I wasn't 
running sync() before dropping the caches. Once I added that, startup time 
became dramatically worse (50% slowdown). Full results here: 
https://gist.github.com/adembo/d4159f9610fc6c7b950bfff9272c0683.

I know you weren't a fan of using extent maps to fix inconsistencies from the 
get go, (and I tip my proverbial cap to your intuition) but I do think it's 
worth preserving this in some form. Without the extent maps, we can't provide 
as good information during a repair as to how much excess space was 
preallocated at the end of a file, or how much space was "unpunched" (when 
that's implemented). So, what do you think of gating the (expensive) repair 
behind a gflag? It'd be off during regular startup (so only "cheap" repairs 
will be done), and on during "kudu fs check".

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Allow forcing color clang diagnostics with an env var

Adar Dembo has posted comments on this change.

Change subject: Allow forcing color clang diagnostics with an env var
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3161d6dcc3715693771089351aadb29810c00fcc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Allow forcing color clang diagnostics with an env var

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Allow forcing color clang diagnostics with an env var
..

Allow forcing color clang diagnostics with an env var

Change-Id: I3161d6dcc3715693771089351aadb29810c00fcc
---
M CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)


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

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

[kudu-CR] gutil: stop supporting AMD Opteron K8

Hello Adar Dembo,

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

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

to review the following change.

Change subject: gutil: stop supporting AMD Opteron K8
..

gutil: stop supporting AMD Opteron K8

This removes a workaround for a bug in very old (circa 2003) Opteron
chips. The workaround was producing an extra branch around most of our
atomic operations. This likely has a negligible perf impact, but it's
possible the slightly smaller code would make us run a bit faster or
reduce branch mispredictions.

This also adds a missing call to init the feature flags in the first
place, which we seem to have forgotten the first time around.

Impala made a similar change[1][2] a while back and seems to have had
no real issues.

[1] https://gerrit.cloudera.org/#/c/2516/
[2]
https://github.com/apache/incubator-impala/commit/a7c3f301bb1da14d6bb35d37aab47450c67cca0c

Change-Id: I59b5aeaae56b5f310b5dd160b868fcb5d04f4da1
---
M src/kudu/gutil/atomicops-internals-tsan.h
M src/kudu/gutil/atomicops-internals-x86.cc
M src/kudu/gutil/atomicops-internals-x86.h
M src/kudu/util/init.cc
4 files changed, 9 insertions(+), 35 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I59b5aeaae56b5f310b5dd160b868fcb5d04f4da1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo

[kudu-CR] Add 'ksck tserver list' tool

Todd Lipcon has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6654/4//COMMIT_MSG
Commit Message:

Line 7: Add 'ksck tserver list' tool
you mean 'kudu tserver list'?


http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 405:   for (int col = 0; col < num_columns; col++) {
nit: indentation


PS4, Line 407: size_t padding = value.size() - widths[col];
 : cout << " " << value;
 : if (col != num_columns - 1) cout << setw(padding) << " 
|";
nit: might make more sense to put the definition of 'padding' inside of the 
body of the if() and use {}


Line 416: void JsonPrintTable(const vector& headers, const 
vector& columns) {
> Why not use JsonWriter? I think that's way less error prone than rolling yo
+1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
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](branch-1.2.x) KUDU-1933. consensus: Avoid and repair integer overflow in log index

Hello David Ribeiro Alves, Kudu Jenkins,

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

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

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

Change subject: KUDU-1933. consensus: Avoid and repair integer overflow in log 
index
..

KUDU-1933. consensus: Avoid and repair integer overflow in log index

We observed a crash on a long-running master server that looked like the
following:

  F0308 00:25:53.568773  7655 log_index.cc:171] Check failed: log_index > 0 
(-2147483648 vs. 0)

It turns out that this was caused due to integer overflow on the log
index field. This patch fixes this type of truncation in a couple of
places (LogReader and MakeOpId()) and adds a couple of new tests that
fail without both of those fixes.

This patch also adds "repair" logic in log replay during tablet
bootstrap that "reverts" integer overflow when it is detected while
rewriting the log entry.

Finally, this patch includes some test helper fixes to avoid log index
integer truncation in future tests.

This backport was modified from the original patch by changing the class
name of the regression test in log-test.cc to LogTest, which is the only
test class available in the 1.2.x code line.

Change-Id: I284edbde51dc50fb2f98acc83cdcc3891d37863f
Reviewed-on: http://gerrit.cloudera.org:8080/6376
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
(cherry picked from commit 8363b74506f8513e2fa9dbf772e30d0abce4e444)
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/opid_util.cc
M src/kudu/consensus/opid_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
11 files changed, 237 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I284edbde51dc50fb2f98acc83cdcc3891d37863f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.2.x
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: simplify MemTracker and move process throttling elsewhere
..

WIP: simplify MemTracker and move process throttling elsewhere

This takes a first step towards simplifying MemTracker:

- Remove the "GC function" callbacks (we never used this)

- Remove the 'ExpandLimits' code which was unimplemented.

- Remove the logging functionality, which we've never used
  as far as I can remember.

- Remove soft limiting. We only used this on the root tracker, so
  I just moved it to a new process_memory.{h,cc}

- Remove 'consumption_func' and un-tie the root tracker from
  the global process memory usage. Now the root tracker is a simple
  sum of its descendents.

Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
12 files changed, 298 insertions(+), 434 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add 'ksck tserver list' tool

Adar Dembo has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 4:

(17 comments)

> @adar - how much testing would you like to see for this commit?

Good question.

I think we want at least some coverage of the printing methods. Let's not worry 
about testing exact output (unless you really want to), but for the machine 
readable ones, can we machine read them and make sure they parse?

As for the new action, let's get some coverage in kudu-tool-test for it (i.e. 
you bring up an external cluster with n tservers and you see n tservers when 
you run the tool).

http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 372: // Pretty print a table.
Maybe a snippet of sample output? Would be nice for the other variants too.


PS4, Line 381:  
Nit: no separation.


Line 416: void JsonPrintTable(const vector& headers, const 
vector& columns) {
Why not use JsonWriter? I think that's way less error prone than rolling your 
own, and safer to extend in the future too. It'll handle escaping too, maybe?


Line 474: .default_rpc_timeout(timeout)
Hmm, you sure we want to use timeout_ms as the RPC timeout as well?


Line 484:   const 
Req&, Resp*,
Nit: indentation.


http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 76: Status SyncLeaderMasterRpc();
Unused and undefined?


Line 118:   // Initialize the leader master proxy given the provided tool 
context.
Should explain what arguments (required/optional) are referenced.


http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

Line 41: 
Nit: since DECLARE_foo are macros, I think they should precede using statements 
(and thus be contiguous w.r.t. #include statements).


PS4, Line 44: DECLARE_bool(show_uuid);
: DECLARE_bool(show_rpc_addresses);
: DECLARE_bool(show_http_addressses);
Where are these used?


Line 96: return Status::IOError(SecureShortDebugString(resp.error()));
We typically extract the Status via StatusFromPB(resp.error().status()).


Line 102:   const auto servers = resp.servers();
Not cref?


PS4, Line 104:  
I think our convention is not to separate the capture from the arguments.

$ git grep '\[\] (' src | wc -l
14
$ git grep '\[\](' src | wc -l
50


PS4, Line 114: push_back
Could be emplace_back?


PS4, Line 124: } else if (boost::iequals(column, "rpc-addresses") ||
 :boost::iequals(column, "rpc_addresses")) {
Why? I don't think the "underscores are the same as dashes" expectation extends 
to gflag _values_, just to _keys_. Do we already do this sort of thing 
elsewhere?


Line 175:   unique_ptr list_tservers =
Nit: indentation isn't quite right w.r.t. the other actions.


Line 178: .ExtraDescription("By default, tablet server UUIDs and RPC 
addresses will be shown. "
So is the idea that different actions that use --columns may have different 
default values for it? I think there's some function you can call to 
programatically change a gflag's  default value. If you used that, you wouldn't 
need any of ExtraDescription() because it'd be obvious from the inclusion of 
--columns' help.

I don't know whether it would be easy to program the default value in the 
context of both running the action and running the tool with --help; could you 
look into it and share what you find?


Line 189:   .Description("Operate on a Kudu Tablet Server")
The new action doesn't match this; it doesn't operate on a single tablet 
server. Perhaps it'd be better suited for the 'cluster' mode?


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

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

[kudu-CR](branch-1.2.x) KUDU-1741: Keep MiniCluster::Restart consistent with ExternalMiniCluster::Restart

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1741: Keep MiniCluster::Restart consistent with 
ExternalMiniCluster::Restart
..


Patch Set 1: Code-Review+1

LGTM

+1 since I suspect the gatekeepers of the 1.2 release are supposed to approve 
it with their +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad33b7c46bfca3f277ccbca7d0420272f06a6633
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.2.x
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR](branch-1.2.x) KUDU-1933. consensus: Avoid and repair integer overflow in log index

Hello David Ribeiro Alves, Kudu Jenkins,

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

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

to review the following change.

Change subject: KUDU-1933. consensus: Avoid and repair integer overflow in log 
index
..

KUDU-1933. consensus: Avoid and repair integer overflow in log index

We observed a crash on a long-running master server that looked like the
following:

  F0308 00:25:53.568773  7655 log_index.cc:171] Check failed: log_index > 0 
(-2147483648 vs. 0)

It turns out that this was caused due to integer overflow on the log
index field. This patch fixes this type of truncation in a couple of
places (LogReader and MakeOpId()) and adds a couple of new tests that
fail without both of those fixes.

This patch also adds "repair" logic in log replay during tablet
bootstrap that "reverts" integer overflow when it is detected while
rewriting the log entry.

Finally, this patch includes some test helper fixes to avoid log index
integer truncation in future tests.

Change-Id: I284edbde51dc50fb2f98acc83cdcc3891d37863f
Reviewed-on: http://gerrit.cloudera.org:8080/6376
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
(cherry picked from commit 8363b74506f8513e2fa9dbf772e30d0abce4e444)
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/opid_util.cc
M src/kudu/consensus/opid_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
11 files changed, 236 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I284edbde51dc50fb2f98acc83cdcc3891d37863f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.2.x
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] pstack watcher: set a limit on backtrace depth

Todd Lipcon has submitted this change and it was merged.

Change subject: pstack_watcher: set a limit on backtrace depth
..


pstack_watcher: set a limit on backtrace depth

On RHEL 6.8, pstack_watcher-test times out fairly reliably, with a 'gdb'
process left spinning 100% CPU.

Some googling found a report of a similar spinning GDB while taking a
backtrace[1]. This report suggests setting a backtrace limit --
apparently the spinning is due to gdb getting in some infinite loop
trying to walk the stack with insufficient/incorrect debug info.

This patch sets such a limit. I verified on a box which was reliably
reproducing the timeout that it is now fixed.

[1] https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/434168

Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Reviewed-on: http://gerrit.cloudera.org:8080/6658
Reviewed-by: Adar Dembo 
Reviewed-by: Mike Percy 
Tested-by: Kudu Jenkins
---
M src/kudu/util/pstack_watcher.cc
1 file changed, 10 insertions(+), 0 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] pstack watcher: set a limit on backtrace depth

Mike Percy has posted comments on this change.

Change subject: pstack_watcher: set a limit on backtrace depth
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] pstack watcher: set a limit on backtrace depth

Adar Dembo has posted comments on this change.

Change subject: pstack_watcher: set a limit on backtrace depth
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] pstack watcher: set a limit on backtrace depth

Hello Mike Percy,

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

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

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

Change subject: pstack_watcher: set a limit on backtrace depth
..

pstack_watcher: set a limit on backtrace depth

On RHEL 6.8, pstack_watcher-test times out fairly reliably, with a 'gdb'
process left spinning 100% CPU.

Some googling found a report of a similar spinning GDB while taking a
backtrace[1]. This report suggests setting a backtrace limit --
apparently the spinning is due to gdb getting in some infinite loop
trying to walk the stack with insufficient/incorrect debug info.

This patch sets such a limit. I verified on a box which was reliably
reproducing the timeout that it is now fixed.

[1] https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/434168

Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
---
M src/kudu/util/pstack_watcher.cc
1 file changed, 10 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] pstack watcher: set a limit on backtrace depth

Todd Lipcon has posted comments on this change.

Change subject: pstack_watcher: set a limit on backtrace depth
..


Patch Set 1:

(2 comments)

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

PS1, Line 14: apparently the spinning is due to gdb getting in some infinite 
loop
: trying to walk the stack with insufficient/incorrect debug info.
> Do we have a reference for this part though? I didn't see that conclusion r
i'm guessing it's that it's not able to interpret the more modern debuginfo or 
something. It's hard to debug (when I tried to gdb gdb things didn't go well) 
and since it's not our bug, I'm satisfied with the workaround.


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

Line 139:   argv.push_back("set backtrace limit 100");
> Could you add a comment explaining why we do this?
will do


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

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

[kudu-CR] pstack watcher: set a limit on backtrace depth

Adar Dembo has posted comments on this change.

Change subject: pstack_watcher: set a limit on backtrace depth
..


Patch Set 1:

(2 comments)

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

PS1, Line 14: apparently the spinning is due to gdb getting in some infinite 
loop
: trying to walk the stack with insufficient/incorrect debug info.
Do we have a reference for this part though? I didn't see that conclusion 
reached in the linked bug report or any of its related bugs.


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

Line 139:   argv.push_back("set backtrace limit 100");
Could you add a comment explaining why we do this?

I think the rest of the commands are self-explanatory, but feel free to also 
comment any that you think may not be.


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

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

[kudu-CR] pstack watcher: set a limit on backtrace depth

Mike Percy has posted comments on this change.

Change subject: pstack_watcher: set a limit on backtrace depth
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] pstack watcher: set a limit on backtrace depth

Hello Mike Percy,

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

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

to review the following change.

Change subject: pstack_watcher: set a limit on backtrace depth
..

pstack_watcher: set a limit on backtrace depth

On RHEL 6.8, pstack_watcher-test times out fairly reliably, with a 'gdb'
process left spinning 100% CPU.

Some googling found a report of a similar spinning GDB while taking a
backtrace[1]. This report suggests setting a backtrace limit --
apparently the spinning is due to gdb getting in some infinite loop
trying to walk the stack with insufficient/incorrect debug info.

This patch sets such a limit. I verified on a box which was reliably
reproducing the timeout that it is now fixed.

[1] https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/434168

Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
---
M src/kudu/util/pstack_watcher.cc
1 file changed, 2 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ide46209256fd4ced6b08065c2ae3c0046de49565
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-463. Add checksumming to cfile
..


Patch Set 3:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6630/3//COMMIT_MSG
Commit Message:

PS3, Line 19: immeditaly
typo


PS3, Line 25: writen
typo


PS3, Line 27: enabled
typo


http://gerrit.cloudera.org:8080/#/c/6630/3/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

PS3, Line 772:   FLAGS_cfile_write_checksums = true;
 :   FLAGS_cfile_verify_checksums = true;
 :   TestReadWriteRawBlocks(NO_COMPRESSION, 1000);
 :   TestReadWriteRawBlocks(SNAPPY, 1000);
 : 
 :   FLAGS_cfile_write_checksums = true;
 :   FLAGS_cfile_verify_checksums = false;
 :   TestReadWriteRawBlocks(NO_COMPRESSION, 1000);
 :   TestReadWriteRawBlocks(SNAPPY, 1000);
 : 
 :   FLAGS_cfile_write_checksums = false;
 :   FLAGS_cfile_verify_checksums = true;
 :   TestReadWriteRawBlocks(NO_COMPRESSION, 1000);
 :   TestReadWriteRawBlocks(SNAPPY, 1000);
 : 
 :   FLAGS_cfile_write_checksums = false;
 :   FLAGS_cfile_verify_checksums = false;
 :   TestReadWriteRawBlocks(NO_COMPRESSION, 1000);
 :   TestReadWriteRawBlocks(SNAPPY, 1000);
maybe this could be collapsed into a for loop?

for (FLAGS_cfile_write_checksum : {false, true}) {
  for (FLAGS_cfile_verify_checksum : {false, true}) {
...
  }
}


PS3, Line 819: CorruptReadableBlock
would it be simpler to just corrupt the data on disk instead of this 
code-injection-based approach? A hook in the fs manager code to corrupt a block 
on disk is probably more generally reusable in higher-level integration tests 
as well (where injecting this CorruptReadableBlock class may be a bit tricky).

I think that would also enable making this code quite a bit simpler by reusing 
existing ReadFile/WriteFile functions


http://gerrit.cloudera.org:8080/#/c/6630/3/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

PS3, Line 157: > IncompatibleFeatures::ALL
this strikes me as odd... don't you mean & (~IncompatibleFeatures::ALL)?


PS3, Line 363: uint32_t
best to use a signed int here


Line 366: data_size = ptr.size() - sizeof(uint32_t);
we should verify that ptr.size() >= sizeof(uint32_t) first and return a 
Corruption otherwise.


Line 392: RETURN_NOT_OK(block_->Read(checksum_offset, sizeof(uint32_t), 
, checksum_scratch));
I think perf wise it may be better to just allocate the extra 4 bytes in the 
block cache so that we can read the checksum in the same IO vs issuing the 
extra syscall here. Plus the code is likely to be a little simpler?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1952 Improve block placement

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1952 Improve block placement
..


Patch Set 1:

(3 comments)

Just took a quick look at the commit message and new configs, not too much on 
the code structure, since Adar said he'd be reviewing the code.

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

Line 10: single-disk failure. Throughout the code, the term "DataDir" refers to
would be good to link to the design doc somewhere here


PS1, Line 14: This patch adds a mapping from tablet to a set of disks and uses 
it to
: replace the existing round-robin placement of blocks. Tablets are
: m
it would be good in the commit message to explain the impact of this patch.

With just this patch, and no follow-up work, is there a negative impact here? 
e.g. are we now limiting tablets to 3 disks, but still not actually able to 
recover from a wider variety of failures? If so, we should probably have a flag 
to disable the new behavior so that our trunk remains non-regressive from the 
most recent release.

Also curious about upgrade impact here.


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

Line 57:   "Indicates the target number of data dirs to spread each 
"
what's the behavior here if the configuration is larger than the number of data 
dirs?

Similar to one of my top-level comments on the commit message: I'm afraid this 
is currently adding additional placement restrictions but not achieving the 
recovery benefits, so we should probably have this off for now


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](branch-1.3.x) docs: Fix "make site"

Todd Lipcon has posted comments on this change.

Change subject: docs: Fix "make site"
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9050eec37ea6b5b4917a4c8269de4178c3c14cb2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [docs] Add another workaround for macOS

Todd Lipcon has posted comments on this change.

Change subject: [docs] Add another workaround for macOS
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I870189eddec0a2e34221b5bbdf85353a91fcf527
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) docs: Fix axis lable for hash-range-partitioning

Todd Lipcon has posted comments on this change.

Change subject: docs: Fix axis lable for hash-range-partitioning
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I619d2082941de257cee4270079b46116944c77a6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: t...@phdata.io
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

Line 80: token_validity_seconds_(2),
> is this going to be flaky in TSAN? or is it OK because the new behavior wil
I changed num_tablet_servers_ to 3 to comply with the restrictions in our code 
to use TestWorkload for better test coverage.

As I see, the heartbeat interval as 10 msec is successfully used in majority of 
generic Kudu tests and it does not cause flakiness.

Short token validity interval should not bring problems here because the client 
is supposed to retry, so I don't expect flakiness in SAN builds, given that 2 
seconds is enough to complete Kudu RPC eventually, even on slow VMs.


PS2, Line 78:   : num_tablet_servers_(2),
: heartbeat_interval_ms_(10),
: token_validity_seconds_(2),
: tsk_rotation_seconds_(1),
: key_column_name_("key"),
: key_split_value_(8) {
> why are all of these members instead of just constants?
Because they are re-used in the code below.  Yes, the tsk_rotation_seconds_ 
become obsolete -- I removed it.


Line 88: //FLAGS_scanner_gc_check_interval_us = 50 * 1000;
> ?
Done


Line 98: CHECK_OK(b.Build(_));
> if you don't care about the schema in particular, can you use one of the ex
Done


PS2, Line 166: size_t
> prefer signed type (int)
Done


PS2, Line 177: NO
> typo
Done


PS2, Line 213:   // Since the authn token is verified upon connection 
establishment, it's
 :   // necessary to make sure the client does not keep the 
connections to
 :   // corresponding tablet servers open. So, let's restart the 
tablet servers
 :   // since the RPC layer might keep already established 
connections open.
> we could set the rpc keepalive timer to short, instead?
Good idea -- I added that.


Line 229: //ASSERT_OK(scanner.SetFaultTolerant());
> ?
Done


Line 230: ASSERT_OK(scanner.Open());
> this is only really testing the case of the token being expired on the begi
Good idea -- I added that for connection negotiations which is just cover some 
edge-cases of the authn token re-acq logic.

I'll extend this to other RPCs as well.


PS2, Line 230: ASSERT_OK(scanner.Open());
 : size_t row_count = 0;
 : while (scanner.HasMoreRows()) {
 :   vector rows;
 :   ASSERT_OK(scanner.NextBatch());
 :   row_count += rows.size();
 : }
 : EXPECT_EQ(2, row_count);
> I think client-test-util has some code to CountRowsFromScanner or something
Thanks, done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..

Allow to pad UNIXTIME_MICROS slots in scan results

This changes the wire protocol to, upon request, pad
the slots that contain values for UNIXTIME_MICROS columns
by 8 bytes. This allows Impala to adopt the result of a
Kudu scan as a whole while still having space to transform
UNIXTIME_MICROS values, which occupy 8 bytes, to the Impala
representation of timestamp, which occupies 16, in place
and without copying memory.

This patch doesn't include any de-serialization logic
the reasoning being that Impala will have knowledge of
the data format in order to perform de-serialization
directly on the "raw" direct and indirect data.

This patch doesn't introduce any branching in the
serialization patch. It does move the memset() call
that was performed once per nullable column, per row,
to be performed on the whole block instead. While less
cache friendly, this is also executed less times. The
net gain is not significant, but it does not regress
in the normal case.

Results of running the wire_protocol-test benchmark
in slow mode:

Before (avg 3 runs): 3.076
After  (avg 3 runs): 3.000

The difference is around -2%, which might be in the noise
but demostrates no significant regression.

Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
---
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
3 files changed, 173 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/6623/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [rpc] handling ERROR UNAVAILABLE RPC error

Hello Kudu Jenkins,

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

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

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

Change subject: [rpc] handling ERROR_UNAVAILABLE RPC error
..

[rpc] handling ERROR_UNAVAILABLE RPC error

This patch adds handling of the newly introduced ERROR_UNAVAILABLE RPC
error code.  The ERROR_UNAVAILABLE error code is a broader counterpart
of the ERROR_SERVER_TOO_BUSY.

>From the client side, both ERROR_UNAVAILABLE and ERROR_SERVER_TOO_BUSY
codes mean it's worth retrying the call at a later time.  To reflect
that, the internal codes {RetriableRpcStatus,ScanRpcStatus}::SERVER_BUSY
are replaced with more generic SERVICE_UNAVAILABLE_TRY_LATER.

Change-Id: I87d780a4ad88c15ceaacfddf6c1b69ed053bb959
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.cc
M src/kudu/rpc/rpc.h
M src/kudu/rpc/rpc_context.h
14 files changed, 267 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/6640/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6640
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I87d780a4ad88c15ceaacfddf6c1b69ed053bb959
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:

PS4, Line 248: *reinterpret_cast(row.mutable_cell_ptr(0)) = i;
 : Slice col1;
 : // See: FillRowBlockWithTestRows() for the reason why we 
relocate these
 : // to 'test_data_arena_'.
 : CHECK(test_data_arena_.RelocateSlice("hello world col1", 
));
 : *reinterpret_cast(row.mutable_cell_ptr(1)) = col1;
 : *reinterpret_cast(row.mutable_cell_ptr(2)) = i;
 : *reinterpret_cast(row.mutable_cell_ptr(3)) = i;
 : row.cell(3).set_null(false);
 : *reinterpret_cast(row.mutable_cell_ptr(4)) = i;
 : row.cell(4).set_null(true);
> would using RowBuilder be simpler here?
this is the method we use for the other tests, so keeping it similar helps to 
understand this one a bit better IMO. plus rowbuilder has  its own arena 
inside, per row, and requires copies.


PS4, Line 298: col3
> col2
Done


PS4, Line 301: bytes.
 : EXPECT_FALSE(BitmapTest(base_data + 68, 3));
 : // 'col4' is supposed to be null, but should also read 0 
since we memsetted the
 : // memory to 0. It should come at 52 byt
> I think these comments should be merged, since the second gives a bit more 
I moved the null bitmap pointer calculation to the top.


http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

PS4, Line 487:   size_t row_size = ContiguousRowHelper::row_size(schema);
> I was actually planning to do this on the next patch up, since this is clie
Done


PS4, Line 691: has_nullable_cols
> Right, but has_nullable_cols only gets set within this if block, which is p
Done


Line 700:   size_t new_base_size = row_stride * num_rows;
> hrm, I would expect this variable to be equal to old_size + row_stride * nu
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

PS4, Line 691: has_nullable_cols
> Right, but has_nullable_cols only gets set within this if block, which is p
ah, you're right. indeed good catch. thanks


http://gerrit.cloudera.org:8080/#/c/6623/6/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

Line 479:   // change any indirect data pointers back to "real" pointers 
instead of
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/6623/6/src/kudu/common/wire_protocol.h
File src/kudu/common/wire_protocol.h:

Line 127: void SerializeRowBlock(const RowBlock& block, RowwiseRowBlockPB* 
rowblock_pb,
> warning: function 'kudu::SerializeRowBlock' has a definition with different
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add 'ksck tserver list' tool

Hello Kudu Jenkins,

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

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

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

Change subject: Add 'ksck tserver list' tool
..

Add 'ksck tserver list' tool

This adds a new tool action to list tablet servers, and associated
information. There are a few output formatting options: the default is a
human-readable table in the psql style; the others are meant for machine
parsing.

The formatting and leader RPC proxy code is abstracted so that
future list tools (e.g. table, tablet, and master) can reuse them.

Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
---
M src/kudu/client/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
5 files changed, 315 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/6654/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6654
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..

Allow to pad UNIXTIME_MICROS slots in scan results

This changes the wire protocol to, upon request, pad
the slots that contain values for UNIXTIME_MICROS columns
by 8 bytes. This allows Impala to adopt the result of a
Kudu scan as a whole while still having space to transform
UNIXTIME_MICROS values, which occupy 8 bytes, to the Impala
representation of timestamp, which occupies 16, in place
and without copying memory.

This patch doesn't include any de-serialization logic
the reasoning being that Impala will have knowledge of
the data format in order to perform de-serialization
directly on the "raw" direct and indirect data.

This patch doesn't introduce any branching in the
serialization patch. It does move the memset() call
that was performed once per nullable column, per row,
to be performed on the whole block instead. While less
cache friendly, this is also executed less times. The
net gain is not significant, but it does not regress
in the normal case.

Results of running the wire_protocol-test benchmark
in slow mode:

Before (avg 3 runs): 3.076
After  (avg 3 runs): 3.000

The difference is around -2%, which might be in the noise
but demostrates no significant regression.

Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
---
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
3 files changed, 166 insertions(+), 33 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread Andrew Wong (Code Review)

Andrew Wong has posted comments on this change.

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

PS4, Line 691:fa
> We're doing that below, right? if padding is not 0 or if there are nullable
Right, but has_nullable_cols only gets set within this if block, which is 
predicated on pad_unixtime_micros_for_impala, so if that's false, even if there 
are nullable columns, has_nullable_cols will never be set!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..


Patch Set 5:

(2 comments)

MJ: Still need to address some comments, but included the client-side fix for 
the pointer re-writing.

http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

PS4, Line 487:   if (pad_unixtime_micros_for_impala) {
> ...and the logic below
I was actually planning to do this on the next patch up, since this is client 
side, but maybe it makes more sense to have it here. I'll move it.


PS4, Line 691:fa
> good catch, agree we need to zero the columns in the case of nulls too.
We're doing that below, right? if padding is not 0 or if there are nullable 
cols we memset the whole thing (that's what the benchmarks on the commit 
message refer to)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to pad UNIXTIME MICROS slots in scan results

2017-04-17 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to pad UNIXTIME_MICROS slots in scan results
..

Allow to pad UNIXTIME_MICROS slots in scan results

This changes the wire protocol to, upon request, pad
the slots that contain values for UNIXTIME_MICROS columns
by 8 bytes. This allows Impala to adopt the result of a
Kudu scan as a whole while still having space to transform
UNIXTIME_MICROS values, which occupy 8 bytes, to the Impala
representation of timestamp, which occupies 16, in place
and without copying memory.

This patch doesn't include any de-serialization logic
the reasoning being that Impala will have knowledge of
the data format in order to perform de-serialization
directly on the "raw" direct and indirect data.

This patch doesn't introduce any branching in the
serialization patch. It does move the memset() call
that was performed once per nullable column, per row,
to be performed on the whole block instead. While less
cache friendly, this is also executed less times. The
net gain is not significant, but it does not regress
in the normal case.

Results of running the wire_protocol-test benchmark
in slow mode:

Before (avg 3 runs): 3.076
After  (avg 3 runs): 3.000

The difference is around -2%, which might be in the noise
but demostrates no significant regression.

Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
---
M src/kudu/common/wire_protocol-test.cc
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
3 files changed, 165 insertions(+), 33 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add 'ksck tserver list' tool

Dan Burkert has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6654/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 87: using consensus::ConsensusServiceProxy;
> warning: using decl 'ConsensusServiceProxy' is unused [misc-unused-using-de
false positive


Line 92: using master::ListTabletServersRequestPB;
> warning: using decl 'ListTabletServersRequestPB' is unused [misc-unused-usi
false positive


Line 93: using master::ListTabletServersResponsePB;;
> warning: using decl 'ListTabletServersResponsePB' is unused [misc-unused-us
false positive


http://gerrit.cloudera.org:8080/#/c/6654/3/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

Line 113:   for (const auto& column : strings::Split(columns_flag, ",")) {
> warning: the variable '__end' is copy-constructed from a const reference bu
ignoring


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

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

[kudu-CR](branch-1.3.x) KUDU-1708. Document Kudu command-line tools

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1708. Document Kudu command-line tools
..


Patch Set 1: Code-Review-1

hm, since this has some code changes, I'm going to defer +2ing it until after 
the 1.3.1 release.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f484f772cbaeb385687d83a2665ae4d7292aaf5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) Add ksck section to admin guide common workflows

Todd Lipcon has posted comments on this change.

Change subject: Add ksck section to admin guide common workflows
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9631337b113d2c67be0057f728c68f792e8a4fd6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [docs] Add security guide

Todd Lipcon has posted comments on this change.

Change subject: [docs] Add security guide
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabf60804975dc105243626be48d3a141c9a4dab5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add 'ksck tserver list' tool

Dan Burkert has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6654/1/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

Line 110: Status PrintTable(vector headers, vector 
columns);
> error: no template named 'vector'; did you mean '__gnu_cxx::vector'? [clang
Done


Line 117:   Status Init(const RunnerContext& init);
> warning: function 'kudu::tools::LeaderMasterProxy::Init' has a definition w
Done


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

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

[kudu-CR] Add 'ksck tserver list' tool

Hello Kudu Jenkins,

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

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

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

Change subject: Add 'ksck tserver list' tool
..

Add 'ksck tserver list' tool

This adds a new tool action to list tablet servers, and associated
information. There are a few output formatting options: the default is a
human-readable table in the psql style; the others are meant for machine
parsing.

The formatting and leader RPC proxy code is abstracted so that
future list tools (e.g. table, tablet, and master) can reuse them.

Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
---
M src/kudu/client/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
5 files changed, 314 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add 'ksck tserver list' tool

Dan Burkert has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6654/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 86: using client::KuduClient;
> warning: using decl 'KuduClient' is unused [misc-unused-using-decls]
Done


Line 396: cout << setfill('-') << setw(widths[col] + 2) << "";
> error: use of undeclared identifier 'setfill'; did you mean 'std::setfill'?
Done


Line 434: void PrintTable(vector columns, string separator) {
> warning: the parameter 'separator' is copied for each invocation but only u
Done


Line 449: Status PrintTable(vector headers, vector 
columns) {
> warning: the parameter 'headers' is copied for each invocation but only use
Done


Line 449: Status PrintTable(vector headers, vector 
columns) {
> warning: the parameter 'columns' is copied for each invocation but only use
Done


http://gerrit.cloudera.org:8080/#/c/6654/1/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

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


Line 55: using client::KuduClient;
> warning: using decl 'KuduClient' is unused [misc-unused-using-decls]
Done


Line 56: using client::sp::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done


Line 60: using rpc::RpcController;
> warning: using decl 'RpcController' is unused [misc-unused-using-decls]
Done


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

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

[kudu-CR] Add 'ksck tserver list' tool

Hello Kudu Jenkins,

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

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

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

Change subject: Add 'ksck tserver list' tool
..

Add 'ksck tserver list' tool

This adds a new tool action to list tablet servers, and associated
information. There are a few output formatting options: the default is a
human-readable table in the psql style; the others are meant for machine
parsing.

The formatting and leader RPC proxy code is abstracted so that
future list tools (e.g. table, tablet, and master) can reuse them.

Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
---
M src/kudu/client/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
5 files changed, 314 insertions(+), 5 deletions(-)


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

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

[kudu-CR] Add 'ksck tserver list' tool

Dan Burkert has posted comments on this change.

Change subject: Add 'ksck tserver list' tool
..


Patch Set 1:

@adar - how much testing would you like to see for this commit?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add 'ksck tserver list' tool

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Add 'ksck tserver list' tool
..

Add 'ksck tserver list' tool

This adds a new tool action to list tablet servers, and associated
information. There are a few output formatting options: the default is a
human-readable table in the psql style; the others are meant for machine
parsing.

The formatting and leader RPC proxy code is abstracted so that
future list tools (e.g. table, tablet, and master) can reuse them.

Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
---
M src/kudu/client/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
5 files changed, 318 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo

[kudu-CR] [rpc] introduced Messenger::reset authn token() method

Alexey Serbin has abandoned this change.

Change subject: [rpc] introduced Messenger::reset_authn_token() method
..


Abandoned

Integrated into https://gerrit.cloudera.org/#/c/6648/

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I3877db4747f977ed1a0b8a2c4f78ca77de7aca93
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [rpc] handling ERROR UNAVAILABLE RPC error

Hello Kudu Jenkins,

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

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

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

Change subject: [rpc] handling ERROR_UNAVAILABLE RPC error
..

[rpc] handling ERROR_UNAVAILABLE RPC error

This patch adds handling of the newly introduced ERROR_UNAVAILABLE RPC
error code.  The ERROR_UNAVAILABLE error code is a broader counterpart
of the ERROR_SERVER_TOO_BUSY.

>From the client side, both ERROR_UNAVAILABLE and ERROR_SERVER_TOO_BUSY
codes mean it's worth retrying the call at a later time.  To reflect
that, the internal codes {RetriableRpcStatus,ScanRpcStatus}::SERVER_BUSY
are replaced with more generic SERVICE_UNAVAILABLE_TRY_LATER.

Change-Id: I87d780a4ad88c15ceaacfddf6c1b69ed053bb959
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.cc
M src/kudu/rpc/rpc.h
M src/kudu/rpc/rpc_context.h
14 files changed, 271 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I87d780a4ad88c15ceaacfddf6c1b69ed053bb959
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [c++ client] re-acquire authn token if expired