date:20170419

Jean-Daniel Cryans has posted comments on this change.

Change subject: [docs] Refresh and augment the know issues
..


Patch Set 1:

Poor rendering: 
https://github.com/jdcryans/kudu/blob/master/docs/known_issues.adoc

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [docs] Refresh and augment the know issues

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: [docs] Refresh and augment the know issues
..

[docs] Refresh and augment the know issues

We've learned a lot about Kudu since people have started using it.
I've gathered in this patch what I think should be the new recommendations
we make to users.

Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
---
M docs/known_issues.adoc
1 file changed, 110 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d8e817a402f419aeb5ed9d700a8207ad9f91e4d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon

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

Adar Dembo has posted comments on this change.

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


Patch Set 6:

(1 comment)

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

Line 189
> Technically this new functionality doesn't even operate on tablet servers -
Sure, but you're missing the gist of my suggestion: the existing description is 
no longer correct and should be updated. What do you think it should be?


-- 
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: 6
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] Add 'kudu 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 (#6).

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

Add 'kudu 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.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
7 files changed, 451 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/6654/6
-- 
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: 6
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

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

Dan Burkert has posted comments on this change.

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


Patch Set 5:

(7 comments)

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

Line 164:   struct Flag {
> Document.
Done


PS5, Line 173:   // Key-value command line arguments. These must actually 
implemented as
 :   // gflags, which means all that must be specified here are the 
gflag names.
 :   // The gflag definitions themselves will be accessed to get 
the argument
 :   // descriptions.
> Update.
Done


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

Line 474:   } else if (boost::iequals(FLAGS_format, "space")) {
> Agreed, but why do we need to override the RPC timeout at all?
I figured it would be nice to have some control over this when writing scripts, 
so you don't always have to wait for the full timeout before it will fail due 
to a wrong master address or similar.


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

Line 426: // Print a table using JSON formatting.
> Snippet? Or some description of the schema?
Done


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

Line 96: 
> Be that as it may, it's more consistent with how we build statuses from err
Done


Line 189
> Can you update the Description then to something like "Operate on Kudu Tabl
Technically this new functionality doesn't even operate on tablet servers - 
it's just contacting the master.


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

Line 101:   for (const auto& column : strings::Split(FLAGS_columns, ",")) {
> Do you want strings::SkipEmpty() here? Then you could omitthe handling of c
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: 5
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] WIP: Expose "raw" mode in KuduScanner and allow to pass flags

Adar Dembo has posted comments on this change.

Change subject: WIP: Expose "raw" mode in KuduScanner and allow to pass flags
..


Patch Set 5:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/6624/5//COMMIT_MSG
Commit Message:

PS5, Line 10: retreived
retrieved


PS5, Line 10: should only
Is this a stylistic choice? Or is it by necessity? Meaning, is it impossible to 
safely retrieve data from a "normal" scanner via direct_data()/indirect_data()?


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2018:   enum { kDefaultRawModeFlags = -1 };
This is confusing. If the idea is to provide a constant that logically means 
"raw mode on but no flags", it should have a value 0 and be part of the 
RawModeFlags enum. But if it means "no raw mode at all"...isn't that the state 
of the world if you don't call EnableRawModeWithFlags()?

Or is this some internal thing? In which case, why is it here at all?


Line 2028: PAD_UNIXTIME_MICROS_TO_16_BYTES = 1
The idea is for these flags not to be mutually exclusive, right? If so, the 
value of the first one should be "1 << 0" so it's clear that the next ones 
should be "1 << 1", "1 << 2", etc. That's how they can be bitflags and OR'ed 
together.


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:

Line 31: using strings::Substitute;
Not used?


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

Line 245:   (configuration().raw_mode_flags() & 
KuduScanner::PAD_UNIXTIME_MICROS_TO_16_BYTES) ==
== shouldn't be necessary.


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/scanners.h
File src/kudu/tserver/scanners.h:

Line 100:   int64_t raw_mode_flags() const { return raw_mode_flags_; }
Why is this a ScannerManager thing? I thought it'd only be per scanner?


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

Line 342:   virtual void set_raw_mode_flags(int64_t raw_mode_flags) {}
> warning: parameter 'raw_mode_flags' is unused [misc-unused-parameters]
If ScanResultCollector is to be a generic "interface" as its class comment 
suggests, it shouldn't provide this default no-op implementation.


Line 410:   void set_raw_mode_flags(int64_t raw_mode_flags) override {
This is unintuitive; since collectors are constructed per RPC, I would have 
expected the right value for pad_unixtime_micros_to_16_bytes_ to be provided 
directly to the constructor. But I think I understand why: the collector is 
stack-allocated before fetching the Scanner, and reversing the order would 
require heap-allocating the collector.

Maybe you can try to explain this chicken-and-egg problem somewhere in the 
collector comments so it's clear for others?


Line 411: if (raw_mode_flags == -1) return;
This special case won't be necessary if the default value is 0 rather than -1.


Line 413: == RawModeFlags::PAD_UNIX_TIME_MICROS_TO_16_BYTES) {
Why is the == necessary? If you mask one bit flag from raw_mode_flags, the 
result will be either non-zero (requested) or zero (not requested).


http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

Line 270:   // "Raw" mode flags.
I understand the desire for a "raw" mode client-side, so that users who sign up 
for timestamp padding are forced to also call  direct_data()/indirect_data() 
instead of row-by-row access.

But why is it exposed to the server? AFAICT, there's no server-side effect. The 
only thing that matters is the timestamp padding, and that can be exposed via 
something like TimestampPaddingMode (similar to ReadMode or OrderMode).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
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] WIP: Expose "raw" mode in KuduScanner and allow to pass flags

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

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Expose "raw" mode in KuduScanner and allow to pass flags
..

WIP: Expose "raw" mode in KuduScanner and allow to pass flags

This adds a way to set a KuduScanner to "raw" mode. In this mode
the data should only be retreived through the direct_data() and
indirect_data() in KuduScanBatch.

It also adds a way to pass "flags" encoded as bits in an int64_t
'raw_mode_flags_' var.

The only use for these flags, presently, is to set
PAD_UNIXTIME_MICROS_TO_16_BYTES, making sure that the server
pads slots for UNIXTIME_MICROS with an additional 8 bytes to
the left.

WIP: Working on a directed test, just making sure I didn't
break anything with the piping.

Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
13 files changed, 160 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
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](gh-pages) FAQ refresh

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: FAQ refresh
..

FAQ refresh

Change-Id: Idd8a79e3067bedbef71b0dcf9921be673e9dfaa0
---
M faq.md
1 file changed, 7 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idd8a79e3067bedbef71b0dcf9921be673e9dfaa0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] cache: add a benchmark

Jean-Daniel Cryans has posted comments on this change.

Change subject: cache: add a benchmark
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6696/1/src/kudu/util/cache-bench.cc
File src/kudu/util/cache-bench.cc:

Line 99:   const int kSecondsToRun = AllowSlowTests() ? 10 : 1;
> yea, though running longer has more chance of triggering races (making this
Can you then document more what this test is about?


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

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

[kudu-CR] cache: add a benchmark

Todd Lipcon has posted comments on this change.

Change subject: cache: add a benchmark
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6696/1/src/kudu/util/cache-bench.cc
File src/kudu/util/cache-bench.cc:

Line 99:   const int kSecondsToRun = AllowSlowTests() ? 10 : 1;
> Is running for a configurable amount of time really making a difference? Yo
yea, though running longer has more chance of triggering races (making this a 
dual-purpose stress test). about to start screwing with the cache to try to 
make it faster and having the stress coverage is nice


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

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

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

Adar Dembo has posted comments on this change.

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


Patch Set 5:

(7 comments)

In case you missed it, I responded to your "how much testing" question earlier 
with some suggestions.

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

Line 164:   struct Flag {
Document.


PS5, Line 173:   // Key-value command line arguments. These must actually 
implemented as
 :   // gflags, which means all that must be specified here are the 
gflag names.
 :   // The gflag definitions themselves will be accessed to get 
the argument
 :   // descriptions.
Update.


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

Line 474:   } else if (boost::iequals(FLAGS_format, "space")) {
> I think it's easier than having two separate timeout options.
Agreed, but why do we need to override the RPC timeout at all?


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

Line 426: // Print a table using JSON formatting.
Snippet? Or some description of the schema?


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

Line 96: 
> That throws away the master error information.
Be that as it may, it's more consistent with how we build statuses from errors 
elsewhere in the CLI.

How about doing what CopyReplica does as a middle ground:

  RETURN_NOT_OK(proxy->StartTabletCopy(req, , ));
  if (resp.has_error()) {
RETURN_NOT_OK_PREPEND(
StatusFromPB(resp.error().status()),
strings::Substitute("Remote server returned error code $0",

TabletServerErrorPB::Code_Name(resp.error().code(;
  }


Line 189
> I originally added it to the cluster mode, but I think it's more discoverab
Can you update the Description then to something like "Operate on Kudu Tablet 
Servers"?


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

Line 101:   for (const auto& column : strings::Split(FLAGS_columns, ",")) {
> warning: the variable '__end' is copy-constructed from a const reference bu
Do you want strings::SkipEmpty() here? Then you could omitthe handling of 
column.empty().


-- 
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: 5
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] make site: only build necessary binaries

Jean-Daniel Cryans has posted comments on this change.

Change subject: make_site: only build necessary binaries
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icbcd5b0327a6419fe732daa05e064aa9249e8298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] cache: add a benchmark

Jean-Daniel Cryans has posted comments on this change.

Change subject: cache: add a benchmark
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6696/1/src/kudu/util/cache-bench.cc
File src/kudu/util/cache-bench.cc:

Line 99:   const int kSecondsToRun = AllowSlowTests() ? 10 : 1;
Is running for a configurable amount of time really making a difference? You'd 
think that once the cache is full that the metrics would converge pretty 
quickly.


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

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

[kudu-CR] make site: only build necessary binaries

Hello Jean-Daniel Cryans,

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

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

to review the following change.

Change subject: make_site: only build necessary binaries
..

make_site: only build necessary binaries

make_site relies on these binaries to build docs for them, but doesn't
rely on any other binaries (or the tests). This speeds up the site
build.

Change-Id: Icbcd5b0327a6419fe732daa05e064aa9249e8298
---
M docs/support/scripts/make_site.sh
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Icbcd5b0327a6419fe732daa05e064aa9249e8298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans

[kudu-CR] log block manager: corruptor test utility

Alexey Serbin has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6582/7/src/kudu/fs/log_block_manager-test-util.cc
File src/kudu/fs/log_block_manager-test-util.cc:

PS7, Line 26: 
nit: it seems  wound be enough in this case.


PS7, Line 322:   for (auto iter = all_containers_.begin(); iter != 
all_containers_.end();) {
 : if (iter->name == c->name) {
 :   all_containers_.erase(iter);
 :   break;
 : } else {
 :   iter++;
 : }
 :   }
nit: would something like

  std::erase(std::remove_if(
  all_containers_.begin(), all_containers_.end()),
  all_containers_.end(),
  [&](const Container& e) { return (c->name == e.name); });

be more idiomatic?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

> If we're considering just unpunched holes, then yes. But, if the container 
> also has space beyond the file size (i.e. both unpunched holes and extra 
> preallocated space), then we don't know to which inconsistency to attribute 
> the excess.

Sure, but if we assume that inconsistencies at all are the exception rather 
than the rule, then we can use the stat check as a first pass, and the only if 
we see a discrepancy, fall back to the more expensive GetExtentMap path?

Also, do we care other than satisfying our curiosity?

-- 
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: 7
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] block manager: gflag to control repairs at startup

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

David Ribeiro Alves has posted comments on this change.

Change subject: block manager: gflag to control repairs at startup
..


Patch Set 1:

(4 comments)

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

PS1, Line 18: As far as the LBM is concerned, 'all' means inconsistency checks 
that need
: extent maps (expensive!) will be performed. Moreover, 'none' 
introduces some
: complexity for fatal and repairable inconsistencies (i.e. partial 
records).
can you provide a some rough numbers on the impact of some, all in terms of 
boostrap time?


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

Line 23: 
should this extra line be here? (i forget)


PS1, Line 51: expensive
maybe we should be more clear regarding what is skipped, at least explain what 
expensive is a little better


PS1, Line 57: static bool ValidateRepair(const char* /*flag_name*/, const 
string& flag_value) {
:   return boost::iequals(flag_value, "all")  ||
:  boost::iequals(flag_value, "some") ||
:  boost::iequals(flag_value, "none");
: }
: DEFINE_validator(block_manager_repair, );
this is cool. gonna crib this for my clock patch.


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

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

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

Dan Burkert has posted comments on this change.

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

Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6640/6/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:

Line 96: FLAGS_rpc_default_keepalive_time_ms = kConnectionKeepaliveMs_;
> As I understand, setting this flag here does set that parameter on the serv
Oh ok, I figured it was an external mini cluster since its an itest. Sounds
good.

Line 175: for (int i = 0; i < num_tablet_servers_; ++i) {
> It seems I forgot to comment on this in the code -- will do.
This isn't quite accurate - the master will return more tablets than requested.
In this case I believe the client never goes back to the master for locations.

Regardless, I think it would be better and simpler to explicitly call a master
RPC if that's the intent - for example ListTables.

Line 279: atomic importer_do_run(true);
> That's mostly for failure scenarios when something goes wrong in the code b
ah, so it's just a short-circuit?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I87d780a4ad88c15ceaacfddf6c1b69ed053bb959
Gerrit-PatchSet: 6
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
Gerrit-HasComments: Yes

[kudu-CR] Silence clang -Waddress-of-packed-member warning

Todd Lipcon has posted comments on this change.

Change subject: Silence clang -Waddress-of-packed-member warning
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6693/1/CMakeLists.txt
File CMakeLists.txt:

Line 249:   # Avoid 'taking address of packed member' warnings, which pollute 
macOs builds.
worth saying "clang 4.0" since I actually noticed this when I tried using 4.0 
on my linux box too.


Line 252:   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} 
-Wno-address-of-packed-member")
should we set this only on concurrent_btree.h with a #pragma push? maybe in 
general it's a good idea? (the cbtree is rather x86-specific anyway in other 
ways)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03518bcca43ea6c891a7f622972465c1ea87ce63
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: corruptor test utility

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

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 7: Code-Review+2

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

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

[kudu-CR] fs: generate report during Open

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

David Ribeiro Alves has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 8: Code-Review+2

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

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

[kudu-CR] Silence clang -Waddress-of-packed-member warning

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

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Silence clang -Waddress-of-packed-member warning
..

Silence clang -Waddress-of-packed-member warning

macOS builds have become polluted with warnings like:

In file included from ../../src/kudu/tablet/deltamemstore.cc:22:
In file included from ../../src/kudu/tablet/deltafile.h:34:
In file included from ../../src/kudu/tablet/deltamemstore.h:33:
../../src/kudu/tablet/concurrent_btree.h:386:33: warning: taking address of 
packed member 'version_' of class or structure 
'kudu::tablet::btree::NodeBase' may result in an 
unaligned pointer value [-Waddress-of-packed-member]
VersionField::SetInserting(_);
^~~~
../../src/kudu/tablet/concurrent_btree.h:721:11: note: in instantiation of 
member function 
'kudu::tablet::btree::NodeBase::SetInserting' 
requested here
this->SetInserting();
  ^
../../src/kudu/tablet/concurrent_btree.h:707:12: note: in instantiation of 
member function 
'kudu::tablet::btree::LeafNode::InsertNew' 
requested here
return InsertNew(mut->idx(), mut->key(), val, mut->arena());
   ^
../../src/kudu/tablet/concurrent_btree.h:1316:19: note: in instantiation of 
member function 
'kudu::tablet::btree::LeafNode::Insert' requested 
here
switch (node->Insert(mutation, val)) {
  ^
../../src/kudu/tablet/concurrent_btree.h:869:19: note: in instantiation of 
member function 
'kudu::tablet::btree::CBTree::Insert' requested 
here
return tree_->Insert(this, val);
  ^
../../src/kudu/tablet/deltamemstore.cc:103:31: note: in instantiation of member 
function 
'kudu::tablet::btree::PreparedMutation::Insert' 
requested here
  if (PREDICT_FALSE(!mutation.Insert(update.slice( {

This is because of clang's -Waddress-of-packed-member warning which has
been committed and reverted and committed again in the clang codebase.

Chromium also silenced this (multiple times), see:
https://bugs.chromium.org/p/chromium/issues/detail?id=637306

Change-Id: I03518bcca43ea6c891a7f622972465c1ea87ce63
---
M CMakeLists.txt
1 file changed, 5 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I03518bcca43ea6c891a7f622972465c1ea87ce63
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves

[kudu-CR] env: add RWFile::GetExtentMap for analyzing file extents

Todd Lipcon has posted comments on this change.

Change subject: env: add RWFile::GetExtentMap for analyzing file extents
..


Patch Set 7: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35bd1bdb9e1a839af2ab95ea73b79217c1f4a2b3
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

Dan Burkert has posted comments on this change.

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


Patch Set 5:

(20 comments)

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

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


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: 
> Maybe a snippet of sample output? Would be nice for the other variants too.
Done


PS4, Line 381: |
> Nit: no separation.
Done


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


PS4, Line 407: if (col != num_columns - 1) cout << "+";
 :   }
 :   cout << endl;
> nit: might make more sense to put the definition of 'padding' inside of the
Done


Line 416:   cout << " " << value;
> Why not use JsonWriter? I think that's way less error prone than rolling yo
Done


Line 474:   } else if (boost::iequals(FLAGS_format, "space")) {
> Hmm, you sure we want to use timeout_ms as the RPC timeout as well?
I think it's easier than having two separate timeout options.


Line 484:   }
> Nit: indentation.
Done


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: // Get the current status of the Kudu server running at 'address', 
storing it
> Unused and undefined?
Done


Line 118:   // Uses the required 'master_addresses' option for the master 
addresses, and
> Should explain what arguments (required/optional) are referenced.
Done


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: using std::string;
> Nit: since DECLARE_foo are macros, I think they should precede using statem
Done


PS4, Line 44: namespace kudu {
: 
: using master::ListTabletServersRequ
> Where are these used?
Done


Line 96: 
> We typically extract the Status via StatusFromPB(resp.error().status()).
That throws away the master error information.


Line 102: headers.emplace_back(column.ToString());
> Not cref?
Done


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


PS4, Line 114: onst auto
> Could be emplace_back?
Done


PS4, Line 124: } else if (boost::iequals(column, "version")) {
 :   for (const auto& server : servers) {
> Why? I don't think the "underscores are the same as dashes" expectation ext
I think it's more ergonomic.  I'm not aware of any other instances that are 
exactly like this.


Line 175:   .Build();
> Nit: indentation isn't quite right w.r.t. the other actions.
Done


Line 178:   .Description("Operate on a Kudu Tablet Server")
> So is the idea that different actions that use --columns may have different
This ended up requiring a little additional plumbing in the tool framework, but 
I think it's a nice additional feature.  GFlag default value and descriptions 
can now be overridden on a per-action basis, which makes them more flexible, 
and easier to write action-specific descriptions.


Line 189
> The new action doesn't match this; it doesn't operate on a single tablet se
I originally added it to the cluster mode, but I think it's more discoverable 
here, and wouldn't require 4 levels of nesting like it would in cluster (kudu 
cluster tserver list).


-- 
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: 5
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] Add 'kudu 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 (#5).

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

Add 'kudu 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.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_tserver.cc
7 files changed, 385 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/6654/5
-- 
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: 5
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

[kudu-CR] log block manager: corruptor test utility

Todd Lipcon has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 7: Code-Review+1

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

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

[kudu-CR] fs: generate report during Open

Todd Lipcon has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 8: Code-Review+1

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

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

[kudu-CR] docs: Add breakpad documentation to user guide

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

Andrew Wong has posted comments on this change.

Change subject: docs: Add breakpad documentation to user guide
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6504/2/docs/troubleshooting.adoc
File docs/troubleshooting.adoc:

PS2, Line 155: so
 : are not usable without access to the exact binary that crashed, 
or a very
 : similar binary.
I might be missing context wrt how minidumps are used. Is this referring to the 
above point that the minidump contains info about processes, shared lib 
versions, etc. or is there another aspect in which the minidumps aren't usable 
in the absence of binaries?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05275aab0196dbf8fc37da320a0bf34662606a14
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] docs: Add breakpad documentation to user guide

Alexey Serbin has posted comments on this change.

Change subject: docs: Add breakpad documentation to user guide
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6504/2/docs/troubleshooting.adoc
File docs/troubleshooting.adoc:

Line 147: 
Does it make sense to add information about enabling/disabling minidumps using 
the ---enable_minidumps flag?  And also mention that it's disabled by default.


PS2, Line 148: glog
nit: why not just 'log' ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05275aab0196dbf8fc37da320a0bf34662606a14
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Todd Lipcon has submitted this change and it was merged.

Change subject: [docs] Add security guide to index
..


[docs] Add security guide to index

Also tweaks the security guide title to better match other pages.

Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Reviewed-on: http://gerrit.cloudera.org:8080/6689
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
(cherry picked from commit 49363aae5e90abaea0118d836b7c34ce4ab7ef53)
Reviewed-on: http://gerrit.cloudera.org:8080/6692
---
M docs/security.adoc
M docs/support/jekyll-templates/document.html.erb
2 files changed, 3 insertions(+), 1 deletion(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] rowset tree-test: protect against divide by zero

Adar Dembo has submitted this change and it was merged.

Change subject: rowset_tree-test: protect against divide by zero
..


rowset_tree-test: protect against divide by zero

A precommit run failed in ASAN with the following error:

  [--] 20 tests from Parameters/TestRowSetTreePerformance
  [ RUN  ] Parameters/TestRowSetTreePerformance.TestPerformance/0
  WARNING: Logging before InitGoogleLogging() is written to STDERR
  I0419 02:42:55.175978 12769 test_util.cc:180] Using random seed: -79660807
  I0419 02:42:55.403628 12769 rowset_tree-test.cc:176] Q=   10 R=   10 
1-by-1 36 ms
  
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/tablet/rowset_tree-test.cc:184:39:
 runtime error: division by zero
  SUMMARY: AddressSanitizer: undefined-behavior 
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/tablet/rowset_tree-test.cc:184:39
 in

It seems incredibly unlikely for batch_total to be 0, but on my laptop I
was able to reproduce this reliably in the first iteration (where the number
of queries and rowsets are 10).

Change-Id: I7033f3242924eabc5e843546bb879a16559a6aed
Reviewed-on: http://gerrit.cloudera.org:8080/6691
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/tablet/rowset_tree-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

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

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

Alexey Serbin has posted comments on this change.

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

Patch Set 6:

(5 comments)

It seems that for the better trust in the test coverage I need to add one more
scenario which would replace the authn token in the client context in the
background (at random times) and after some time import that key into the
master as well, while running some
create-table-write-then-read-the-written-data-drop-the-table workload.

I'll address your comments and add that scenario as well in the next version of
this patch.

http://gerrit.cloudera.org:8080/#/c/6640/6//COMMIT_MSG
Commit Message:

PS6, Line 16: SERVICE_UNAVAILABLE_TRY_LATER.
> SERVICE_UNAVAILABLE
Done

http://gerrit.cloudera.org:8080/#/c/6640/6/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:

Line 96: FLAGS_rpc_default_keepalive_time_ms = kConnectionKeepaliveMs_;
> Doesn't this need to be set on the server?
As I understand, setting this flag here does set that parameter on the server
side as well since the MiniCluster is used here (not ExternalMiniCluster).

I'll add a comment.

Line 175: for (int i = 0; i < num_tablet_servers_; ++i) {
> Why did you add multiple tablets? Is there something different about the m
It seems I forgot to comment on this in the code -- will do.

The idea was to have a scenario where client make calls to both the master and
tablet servers. As I understand, the client populates its meta-cache with the
information on tablet servers it needs to contact to perform the specified
operation, not all the tablets for the specified table. In the beginning of
the test the client makes a single insert into the first tablet with range
[-inf, 0) and stores the info about that. Doing so allows to send write
requests to the tablet server (not the the master) later on while trying to
insert the same value. From the other side, it's necessary to exercise
client-->master calls while populating the meta-cache in the context of
inserting a value in the other, non-yet-discovered tablet. BTW, it seems I
missed one piece there as well: trying to insert into another tablet while
having the original token replaced. I will fix that.

Line 261: // The client automatically retries on getting ServiceUnavailable
from the
> It looks like this retry code may be write-path specific. There is coverag
Yep, there is a scan attempt as well, however it covers the client->master
path. I need to update it to cover the client->tablet server path as well.

Line 279: atomic importer_do_run(true);
> This is pretty confusing. It seems like this bool won't get flipped till t
That's mostly for failure scenarios when something goes wrong in the code below
(like Apply() returns an error).

I'll add a comment.

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

[kudu-CR] env: add RWFile::GetExtentMap for analyzing file extents

Adar Dembo has posted comments on this change.

Change subject: env: add RWFile::GetExtentMap for analyzing file extents
..


Patch Set 7: Verified+1

Filed KUDU-1976 for the unrelated failure.

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

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

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

(3 comments)

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

Line 819:   unique_ptr corrupt_source(
> I will look into this. I think ideally I would like to corrupt each byte in
yea, you probably need to re-open the file and clear the block cache in between


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

Line 363:   uint32_t data_size = ptr.size();
> ptr.size() returns uint32_t, so I kept it the same for consistency.
yea, though the style guide says to use signed quantities anywhere that you 
aren't doing bitwise ops (that way overflow will be more obvious since it will 
go to -1 instead of a very large number)


Line 392: RETURN_NOT_OK(block_->Read(checksum_offset, sizeof(uint32_t), 
, checksum_scratch));
> I didn't want to cache the checksum since that seamed to complicate cache r
hm, possibly, but am worried that doubling the syscalls is expensive


-- 
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Todd Lipcon has uploaded a new change for review.

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

Change subject: [docs] Add security guide to index
..

[docs] Add security guide to index

Also tweaks the security guide title to better match other pages.

Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Reviewed-on: http://gerrit.cloudera.org:8080/6689
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
(cherry picked from commit 49363aae5e90abaea0118d836b7c34ce4ab7ef53)
---
M docs/security.adoc
M docs/support/jekyll-templates/document.html.erb
2 files changed, 3 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert

[kudu-CR] [docs] Add security guide to index

Todd Lipcon has submitted this change and it was merged.

Change subject: [docs] Add security guide to index
..


[docs] Add security guide to index

Also tweaks the security guide title to better match other pages.

Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Reviewed-on: http://gerrit.cloudera.org:8080/6689
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M docs/security.adoc
M docs/support/jekyll-templates/document.html.erb
2 files changed, 3 insertions(+), 1 deletion(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [docs] Add security guide to index

Todd Lipcon has posted comments on this change.

Change subject: [docs] Add security guide to index
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] log block manager: corruptor test utility

Adar Dembo has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 7: Verified+1

Unrelated failure for which I pushed http://gerrit.cloudera.org:8080/6691.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 7
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rowset tree-test: protect against divide by zero

Todd Lipcon has posted comments on this change.

Change subject: rowset_tree-test: protect against divide by zero
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] rowset tree-test: protect against divide by zero

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: rowset_tree-test: protect against divide by zero
..

rowset_tree-test: protect against divide by zero

A precommit run failed in ASAN with the following error:

  [--] 20 tests from Parameters/TestRowSetTreePerformance
  [ RUN  ] Parameters/TestRowSetTreePerformance.TestPerformance/0
  WARNING: Logging before InitGoogleLogging() is written to STDERR
  I0419 02:42:55.175978 12769 test_util.cc:180] Using random seed: -79660807
  I0419 02:42:55.403628 12769 rowset_tree-test.cc:176] Q=   10 R=   10 
1-by-1 36 ms
  
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/tablet/rowset_tree-test.cc:184:39:
 runtime error: division by zero
  SUMMARY: AddressSanitizer: undefined-behavior 
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/tablet/rowset_tree-test.cc:184:39
 in

It seems incredibly unlikely for batch_total to be 0, but on my laptop I
was able to reproduce this reliably in the first iteration (where the number
of queries and rowsets are 10).

Change-Id: I7033f3242924eabc5e843546bb879a16559a6aed
---
M src/kudu/tablet/rowset_tree-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

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

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

Dan Burkert has posted comments on this change.

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

Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6640/6//COMMIT_MSG
Commit Message:

PS6, Line 16: SERVICE_UNAVAILABLE_TRY_LATER.
SERVICE_UNAVAILABLE

http://gerrit.cloudera.org:8080/#/c/6640/4/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:

Line 128: // client-->tserver RPCs.
> It seems there isn't 'bool operator()' for boost:optional. However, there
Oh sorry about that, I thought there was an implicit conversion to bool. SGTM.

http://gerrit.cloudera.org:8080/#/c/6640/6/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:

Line 96: FLAGS_rpc_default_keepalive_time_ms = kConnectionKeepaliveMs_;
Doesn't this need to be set on the server?

Line 175: for (int i = 0; i < num_tablet_servers_; ++i) {
Why did you add multiple tablets? Is there something different about the
multiple tablet case?

Line 261: // The client automatically retries on getting ServiceUnavailable
from the
It looks like this retry code may be write-path specific. There is coverage of
the metadata paths via table create. Perhaps it's worth trying a table scan in
here as well to make sure that's retrying?

Line 279: atomic importer_do_run(true);
This is pretty confusing. It seems like this bool won't get flipped till the
method scope ends, but for that to happen, it has to pass 500ms anyway.

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

[kudu-CR] [docs] Add security guide to index

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: [docs] Add security guide to index
..

[docs] Add security guide to index

Also tweaks the security guide title to better match other pages.

Change-Id: I82091f86e90ed34ed804d0f7bfdca785cfe27e30
---
M docs/security.adoc
M docs/support/jekyll-templates/document.html.erb
2 files changed, 3 insertions(+), 1 deletion(-)


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

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

[kudu-CR](branch-1.3.x) [docs] Revise disk-failure-recovery workflow

Dan Burkert has submitted this change and it was merged.

Change subject: [docs] Revise disk-failure-recovery workflow
..


[docs] Revise disk-failure-recovery workflow

Missed a few comments in the original commit.

Change-Id: I35c2ca56fdcf07917e7e0ba7f0d9be7b6bb19568
Reviewed-on: http://gerrit.cloudera.org:8080/6608
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
(cherry picked from commit 4649cd8af9c1fb3867639fa75d89ac1bddd35f66)
Reviewed-on: http://gerrit.cloudera.org:8080/6688
---
M docs/administration.adoc
1 file changed, 6 insertions(+), 6 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I35c2ca56fdcf07917e7e0ba7f0d9be7b6bb19568
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] docs: Add breakpad documentation to user guide

Todd Lipcon has posted comments on this change.

Change subject: docs: Add breakpad documentation to user guide
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6504/2/docs/troubleshooting.adoc
File docs/troubleshooting.adoc:

PS2, Line 162: Note that while a minidump does not contain a
 : heap memory dump, it does contain stack memory and therefore it 
is possible for
 : application-specific information to appear in a minidump.
move this to a NOTE: section (adoc has special syntax for it).

Also I think "application-specific" is not quite clear. Perhaps "user data" or 
"application data", and add another sentence saying something like "If 
confidential or personal information is stored on the cluster, do not share 
minidump files." to make it extra explicit


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05275aab0196dbf8fc37da320a0bf34662606a14
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [rpc-test] fixed TestConnectionKeepalive flakiness

Alexey Serbin has submitted this change and it was merged.

Change subject: [rpc-test] fixed TestConnectionKeepalive flakiness
..


[rpc-test] fixed TestConnectionKeepalive flakiness

After introducing TLS option for Kudu RPC, the TestConnectionKeepalive
became a bit flaky, failing time to time with output like the following:

[ RUN  ] OptionalSSL/TestRpc.TestConnectionKeepalive/1
I0419 01:31:19.019090  4863 rpc-test.cc:223] Connecting to 0.0.0.0:52055
src/kudu/rpc/rpc-test.cc:225: Failure
Failed
Bad status: Network error: Recv() got EOF from remote (error 108)

[ RUN  ] OptionalSSL/TestRpc.TestConnectionKeepalive/1
I0419 01:31:25.609285   370 rpc-test.cc:223] Connecting to 0.0.0.0:41418
W0419 01:31:25.853305   398 connection.cc:462] client connection to \
  0.0.0.0:41418 recv error: Network error: recv error: \
  Connection reset by peer (error 104)
src/kudu/rpc/rpc-test.cc:225: Failure
Failed
Bad status: Network error: recv error: Connection reset by peer (error 104)

It seems the TLS connection establishment phase in some cases takes too long,
so by the time of test RPC call might close.

Bumping the keepalive interval helped to address the issue.  Prior to
this fix, if running with --stress_cpu_threads=32 option,
usually 15 out of 1024 runs failed.  After the fix 0 out of 4096 runs failed.

Change-Id: I8fb2dec953c1047ea43fe9e28d470ae6566077c5
Reviewed-on: http://gerrit.cloudera.org:8080/6683
Tested-by: Kudu Jenkins
Reviewed-by: Sailesh Mukil 
Reviewed-by: Dan Burkert 
---
M src/kudu/rpc/rpc-test.cc
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Sailesh Mukil: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8fb2dec953c1047ea43fe9e28d470ae6566077c5
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: Mike Percy 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] docs: Shorten top-level headings in Troubleshooting Guide

Todd Lipcon has posted comments on this change.

Change subject: docs: Shorten top-level headings in Troubleshooting Guide
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iccd0daf7954f79760a20f1fd281c3d167114a063
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
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

Todd Lipcon has posted comments on this change.

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


Patch Set 7:

> Right, we talked about this before. Assuming an "expensive repair" thing 
> exists, the issue isn't knowing _when_ to re-hole-punch; it's knowing _how 
> much_ unpunched space exists (for reporting purposes), and this approach 
> doesn't provide an accurate measure of that.

Still missing something. Isn't that stat.st_blocks * stat.st_blksize - 
expected_size?

-- 
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: 7
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] [rpc-test] fixed TestConnectionKeepalive flakiness

Dan Burkert has posted comments on this change.

Change subject: [rpc-test] fixed TestConnectionKeepalive flakiness
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8fb2dec953c1047ea43fe9e28d470ae6566077c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [docs] Revise disk-failure-recovery workflow

Todd Lipcon has posted comments on this change.

Change subject: [docs] Revise disk-failure-recovery workflow
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35c2ca56fdcf07917e7e0ba7f0d9be7b6bb19568
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] Revise disk-failure-recovery workflow

Dan Burkert has uploaded a new change for review.

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

Change subject: [docs] Revise disk-failure-recovery workflow
..

[docs] Revise disk-failure-recovery workflow

Missed a few comments in the original commit.

Change-Id: I35c2ca56fdcf07917e7e0ba7f0d9be7b6bb19568
Reviewed-on: http://gerrit.cloudera.org:8080/6608
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
(cherry picked from commit 4649cd8af9c1fb3867639fa75d89ac1bddd35f66)
---
M docs/administration.adoc
1 file changed, 6 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I35c2ca56fdcf07917e7e0ba7f0d9be7b6bb19568
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert

[kudu-CR](branch-1.3.x) [docs] Add admin workflow for recovering from disk failure

Dan Burkert has posted comments on this change.

Change subject: [docs] Add admin workflow for recovering from disk failure
..


Patch Set 1:

Going to verify since only doc change, and I'm waitin gon it

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

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

[kudu-CR](branch-1.3.x) [docs] Add admin workflow for recovering from disk failure

Dan Burkert has posted comments on this change.

Change subject: [docs] Add admin workflow for recovering from disk failure
..


Patch Set 1: Verified+1

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

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

[kudu-CR](branch-1.3.x) [docs] Add admin workflow for recovering from disk failure

Dan Burkert has submitted this change and it was merged.

Change subject: [docs] Add admin workflow for recovering from disk failure
..


[docs] Add admin workflow for recovering from disk failure

I didn't document how to rebalance tablets onto the repaired tserver if
necessary, since the process is complicated and error prone, and we hope
to have a rebalancing tool in the future. These docs will quickly become
outdated when KUDU-616 is fixed, but I think it's worth it to document
since we frequently receive questions on the topic.

Change-Id: I6541bffc5e9546c523df610fd8c025dd05e403bf
Reviewed-on: http://gerrit.cloudera.org:8080/6606
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Andrew Wong 
(cherry picked from commit 87154f4a39c77ab92d80f3effa58de3000921127)
Reviewed-on: http://gerrit.cloudera.org:8080/6677
Reviewed-by: Hao Hao 
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Dan Burkert 
---
M docs/administration.adoc
1 file changed, 35 insertions(+), 0 deletions(-)

Approvals:
  Dan Burkert: Verified
  Jean-Daniel Cryans: Looks good to me, approved
  Hao Hao: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6541bffc5e9546c523df610fd8c025dd05e403bf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

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

Jean-Daniel Cryans has submitted this change and it was merged.

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


KUDU-1708. Document Kudu command-line tools

- Adds helpxml flag support to the kudu binary
- Adds doc script and xslt to convert xml to asciidoc

Change-Id: I9f484f772cbaeb385687d83a2665ae4d7292aaf5
Reviewed-on: http://gerrit.cloudera.org:8080/6525
Reviewed-by: Adar Dembo 
Tested-by: Adar Dembo 
(cherry picked from commit 2407c2cb1e413c705ceb2b9479fe74d9cc432883)
Reviewed-on: http://gerrit.cloudera.org:8080/6645
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
A docs/command_line_tools_reference.adoc
M docs/support/jekyll-templates/document.html.erb
M docs/support/scripts/make_docs.sh
A docs/support/xsl/tool_to_asciidoc.xsl
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/tool_action-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_main.cc
10 files changed, 496 insertions(+), 24 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9f484f772cbaeb385687d83a2665ae4d7292aaf5
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

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

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 2: Code-Review+2

-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: 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) [docs] Add admin workflow for recovering from disk failure

Jean-Daniel Cryans has posted comments on this change.

Change subject: [docs] Add admin workflow for recovering from disk failure
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR](gh-pages) Add blog post for 1.3.1

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: Add blog post for 1.3.1
..


Add blog post for 1.3.1

Change-Id: If8a94c8b75d4ba39fdee151e775a65ed34f5ee79
Reviewed-on: http://gerrit.cloudera.org:8080/6685
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Jean-Daniel Cryans 
---
A _posts/2017-04-19-apache-kudu-1-3-1-released.md
1 file changed, 19 insertions(+), 0 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If8a94c8b75d4ba39fdee151e775a65ed34f5ee79
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans

[kudu-CR](gh-pages) Add blog post for 1.3.1

Jean-Daniel Cryans has posted comments on this change.

Change subject: Add blog post for 1.3.1
..


Patch Set 1: Code-Review+2 Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If8a94c8b75d4ba39fdee151e775a65ed34f5ee79
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

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

2017-04-19 Thread Grant Henke (Code Review)

Hello Kudu Jenkins,

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

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

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

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

WIP: KUDU-463. Add checksumming to cfile

Adds optional checksumming and validation to cfile blocks.

Introduces 2 flags to control cfile checksumming:
- cfile_write_checksums (default false)
- cfile_verify_checksums (default true)

cfile_write_checksums is used in the CFileWriter to enable computing and
appending Crc32 checksums to the end of each cfile block, header and footer

cfile_write_checksums is defaulted to false to ensure upgrades don't
immediately result in performance degredation and incompatible data on
downgrade. It can and likely should be defaulted to true in a later release.

When cfile_write_checksums is set to true, the existing incompatible_features
bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear
error message when older versions of Kudu try to read the file. If checksums
are not written the incompatible_features "checksum" bit is not set.

cfile_verify_checksums is used in the CFileReader to enable validating the
data against the written checksum. cfile_verify_checksums is defaulted to
true since validation only occurs if checksums are written. Any data that
was written before checksumming was an option or when cfile_write_checksums
was false will not be verified.

TODO:
- Finish testing
- Change cfile block to page to avoid term overload?

Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
---
M docs/design-docs/cfile.md
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/util/crc.cc
M src/kudu/util/crc.h
9 files changed, 346 insertions(+), 46 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
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 (#6).

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.

Added an integration test to cover the behavior of the server components
and Kudu C++ client when client sends authn token signed by a TSK
unknown to the master or tablet server.

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, 380 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/6640/6
-- 
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: 6
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] [rpc] handling ERROR UNAVAILABLE RPC error

Alexey Serbin has posted comments on this change.

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


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6640/4/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:

Line 50: //DECLARE_int32(heartbeat_rpc_timeout_ms);
> remove these?
Done


PS4, Line 83: torun
> typo
Done


Line 128:   ASSERT_NE(boost::none, authn_token);
> I think this can be simplified to
It seems there isn't 'bool operator()' for boost:optional.  However, there is 
'bool operator!()' and 'bool is_initialized()'.

So, it seems the choice here is between

ASSERT_FALSE(!authn_token);
  and
ASSERT_TRUE(authn_token.is_initialized());

However, to me
  ASSERT_NE(boost::none, authn_token);
looks better.  I would like to keep this, if you don't mind.


Line 152:   ASSERT_OK(table_creator->table_name(kTableName)
> When I initially read this, I got confused because I expected this to fail 
Thank you for pointing at that.

Yes, exactly -- the connection to the master is cached and any further calls to 
the master would not try to validate the authn token if the connection kept 
open.

Probably, I could set keepalive property for the connections to some small 
value and then make sure the call to the master returns ServiceUnavailable 
error.

I'll do that and add corresponding comment: that way it will cover 
client-->master RPC path as well and it would be less confusing to readers, IMO.


Line 174:   auto cleanup = MakeScopedCleanup([&]() {
> Might be simpler to use ElementDeleter from stl_util.h
Done


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

Gerrit-MessageType: comment
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 
Gerrit-HasComments: Yes

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