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

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

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

This patch wires the new GetExtentMap() method into the LBM for use in
determining whether there exists allocated disk space beyond the container
data file's logical size. Why bother? Well, it's not possible for the
"full container preallocation" accounting in FsReport to be accurate
otherwise. Also, reducing the number of cases wherein containers are
truncated ought to be safer. Finally, we'll eventually use the extent map
for other purposes (e.g. repunching holes efficiently at startup).

My first implementation required LBM users to have GetExtentMap()-compatible
systems. Testing showed this to be safe across Kudu's supported distro and
filesystem matrix. The notable exception was aufs, used by Docker containers
that run our pre-commit tests. It would be unfortunate if Kudu were unusable
in Docker [1], so I adjusted the behavior to be more forgiving.

I also snuck in some cosmetic changes to a few error messages.

1. Though isn't KUDU-1419 still an issue? Maybe not in all versions of aufs?

Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
3 files changed, 110 insertions(+), 20 deletions(-)


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

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

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: fs: generate report during Open
..

fs: generate report during Open

This patch modifies the FsManager and block managers to produce a filesystem
report when opened. The report includes various filesystem statistics as
well as all inconsistencies found and repaired.

The heart of the patch is the FsReport nested data structure. Originally
implemented as a protobuf to take advantage of DebugString(), I found it to
be a poor fit once I began customizing the log output, so I rewrote it in
its current form. The report includes fs-wide statistics as well as optional
consistency checks that may or may not be performed by the block managers.
Reports can be merged; the LBM takes advantage of this as it processes data
directories in parallel. The consistency checks have structure, so as to
simplify testing and ToString() customization.

Thanks to the level of detail in the FsReport, the LBM now separates the act
of finding inconsistencies from repairing them. This makes it much easier to
enforce that repairs aren't done on a read-only filesystem, or if there were
any fatal and irreparable errors.

Thorough testing of the inconsistencies in the report is deferred to a
different patch as this one was already quite large.

Other changes of note:
- The LBM treats data files without matching metadata files as incomplete
  containers; previously only metadata files without matching data files
  were treated in this way.
- The LBM maps all containers by name so they can be looked up during the
  repair process; previously they were stored in a vector.
- The checks performed by "kudu fs check" are in FsReport. I think this
  makes sense, and it's an acknowledgement that they should be performed by
  the block manager in the future. The code in tool_action_fs.cc was
  restructured to write its findings into the report before logging it.

Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
A src/kudu/fs/fs_report.cc
A src/kudu/fs/fs_report.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/util/pb_util.cc
17 files changed, 1,074 insertions(+), 212 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 3
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 

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

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

log block manager: corruptor test utility

This patch introduces the LBMCorruptor, a test-only class for adding various
inconsistencies to the LBM's on-disk structures. The bulk of the patch is a
set of new tests that exercise the corruptor and then verify the results via
the generated FsReport.

Additionally, block_manager-stress-test has been modified to inject
non-fatal inconsistencies via the LBMCorruptor in between passes.

Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
A src/kudu/fs/log_block_manager-test-util.cc
A src/kudu/fs/log_block_manager-test-util.h
M src/kudu/fs/log_block_manager-test.cc
5 files changed, 827 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 3
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 

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

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

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

This patch wires the new GetExtentMap() method into the LBM for use in
determining whether there exists allocated disk space beyond the container
data file's logical size. Why bother? Well, it's not possible for the
"full container preallocation" accounting in FsReport to be accurate
otherwise. Also, reducing the number of cases wherein containers are
truncated ought to be safer. Finally, we'll eventually use the extent map
for other purposes (e.g. repunching holes efficiently at startup).

My first implementation required LBM users to have GetExtentMap()-compatible
systems. Testing showed this to be safe across Kudu's supported distro and
filesystem matrix. The notable exception was aufs, used by Docker containers
that run our pre-commit tests. It would be unfortunate if Kudu were unusable
in Docker [1], so I adjusted the behavior to be more forgiving.

I also snuck in some cosmetic changes to a few error messages.

1. Though isn't KUDU-1419 still an issue? Maybe not in all versions of aufs?

Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
3 files changed, 92 insertions(+), 20 deletions(-)


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

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

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

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

log block manager: corruptor test utility

This patch introduces the LBMCorruptor, a test-only class for adding various
inconsistencies to the LBM's on-disk structures. The bulk of the patch is a
set of new tests that exercise the corruptor and then verify the results via
the generated FsReport.

Additionally, block_manager-stress-test has been modified to inject
non-fatal inconsistencies via the LBMCorruptor in between passes.

Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
A src/kudu/fs/log_block_manager-test-util.cc
A src/kudu/fs/log_block_manager-test-util.h
M src/kudu/fs/log_block_manager-test.cc
5 files changed, 827 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 2
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 

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: fs: generate report during Open
..

fs: generate report during Open

This patch modifies the FsManager and block managers to produce a filesystem
report when opened. The report includes various filesystem statistics as
well as all inconsistencies found and repaired.

The heart of the patch is the FsReport nested data structure. Originally
implemented as a protobuf to take advantage of DebugString(), I found it to
be a poor fit once I began customizing the log output, so I rewrote it in
its current form. The report includes fs-wide statistics as well as optional
consistency checks that may or may not be performed by the block managers.
Reports can be merged; the LBM takes advantage of this as it processes data
directories in parallel. The consistency checks have structure, so as to
simplify testing and ToString() customization.

Thanks to the level of detail in the FsReport, the LBM now separates the act
of finding inconsistencies from repairing them. This makes it much easier to
enforce that repairs aren't done on a read-only filesystem, or if there were
any fatal and irreparable errors.

Thorough testing of the inconsistencies in the report is deferred to a
different patch as this one was already quite large.

Other changes of note:
- The LBM treats data files without matching metadata files as incomplete
  containers; previously only metadata files without matching data files
  were treated in this way.
- The LBM maps all containers by name so they can be looked up during the
  repair process; previously they were stored in a vector.
- The checks performed by "kudu fs check" are in FsReport. I think this
  makes sense, and it's an acknowledgement that they should be performed by
  the block manager in the future. The code in tool_action_fs.cc was
  restructured to write its findings into the report before logging it.

Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
A src/kudu/fs/fs_report.cc
A src/kudu/fs/fs_report.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/util/pb_util.cc
17 files changed, 1,073 insertions(+), 211 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 2
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 

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

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

env: add RWFile::GetExtentMap for analyzing file extents

This patch introduces a method to get the extent metadata of a file provided
it resides on an extent-based filesystem (such as ext4 or xfs). Each extent
is an offset and length into the file, and represents a chunk of filesystem
that has been allocated for the file. Gaps between extents are expected to
be unallocated and may represent punched out holes.

On Linux, the extent listing is retrieved via repeated calls to the
FS_IOC_FIEMAP ioctl, though only some of the information returned is
actually used.

I intend to use this in the log block manager for two purposes:
- Discovering whether a container has allocated space beyond its file size.
- Finding all of a container's "unpunched" holes, and repunching them.

Change-Id: I35bd1bdb9e1a839af2ab95ea73b79217c1f4a2b3
---
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
4 files changed, 157 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35bd1bdb9e1a839af2ab95ea73b79217c1f4a2b3
Gerrit-PatchSet: 2
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 

[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems

[Adar Dembo (Code Review)]

Adar Dembo has abandoned this change.

Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
..


Abandoned

It turns out that aufs does not support FS_IOC_FIEMAP. That's why all of the 
precommit tests failed: aufs is used by Docker containers.

Rather than force Docker users to work around this, I'll make the use of 
FS_IOC_FIEMAP in the LBM more opportunistic.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
Gerrit-PatchSet: 1
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 

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

PS1, Line 875:   char buf[1024];
 :   memset(buf, 0, sizeof(buf));
> I think it's possible to write 
No longer an issue; I'm now going to preallocate the file instead of writing 
zeroes to it.


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

PS1, Line 728: OVERRIDE
> nit: consider replacing with C++11-ish
Just trying to be consistent with the rest of the file.


PS1, Line 737: unique_ptr buf(new char[kBufSize]);
> Is it crucial to allocate this on the heap?  Would stack allocated array su
I was worried about stack allocating a large buffer (say, a megabyte). But I 
suppose 4K isn't too bad.


PS1, Line 747: ~(0ULL)
> Would using FIEMAP_MAX_OFFSET macro be more idiomatic?
Ah neat, didn't know about that.


PS1, Line 749:   int ret = ioctl(fd_, FS_IOC_FIEMAP, fm);
 :   if (ret == -1) {
> nit: since the 'ret' variable is not referenced anywhere else, consider
Done


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

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 1:

(9 comments)

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

Line 7: fs: generate report during Open
> do you have any measurement of whether this affects startup time on a syste
I don't expect it to, so no. There is no additional IO or system calls, and on 
a steady-state node (i.e. no inconsistencies), there are no allocations in the 
report either.

If you're alluding to the use of FS_IOC_FIEMAP on container files at startup, 
that's in a different patch. :)


PS1, Line 32:  previously only the opposite was true.
> what is the opposite?
metadata files without matching data files. I'll reword.


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

Line 65:it = missing_block_by_tablet_id.equal_range(it->first).second) {
> Do we ever expect this to be a big map?
Missing blocks are fatal, so no, I don't expect there to be any. Put another 
way, I would expect none, or all (in the event of someone wiping their data 
directories but preserving superblocks), and optimizing for the latter isn't 
that useful.

I was trying to hard to emulate Guava's MultiMap.asMap(), but I agree that this 
is pretty inscrutable. I'll rework into map.


PS1, Line 68: bool first = true;
: for (auto it = range.first; it != range.second; it++) {
:   if (first) {
: first = false;
:   } else {
: block_str += ", ";
:   }
:   block_str += it->second;
: }
> if you did the map> above, you could save a bunch of LOC here
Done


Line 78: s += Substitute("Fatal error: tablet $0 missing blocks: [ $1 ]\n",
> can use SubstituteAndAppend
Done


Line 228: s += Substitute("Misaligned block in container $0: $1\n",
> SubstituteAndAppend
Done


PS1, Line 324:   if (malformed_record_check && other.malformed_record_check) {
 : 
malformed_record_check->MergeFrom(other.malformed_record_check.get());
 :   } else if (other.malformed_record_check) {
 : malformed_record_check = other.malformed_record_check;
 :   }
> hrm, this is very repetitive. you think it's worth doing a templated functi
I'll do a macro. In ToString() too.


Line 405:   LOG(INFO) << ToString();
> LOG has the downisde of a maximum 32kb (maybe 64kb?) limit, which I could s
Think I should log each line individually? That makes the result a little 
tougher to cut and paste without also removing all the log prefixes.


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

Line 44: Entry(BlockId block_id, std::string tablet_id);
> if this is just a simple assignment constructor, maybe it's not necessary a
Doesn't work with emplace_back(), unfortunately: 
http://stackoverflow.com/questions/13812703/c11-emplace-back-on-vectorstruct?noredirect=1=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: 1
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: Yes

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

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

PS1, Line 875:   char buf[1024];
 :   memset(buf, 0, sizeof(buf));
I think it's possible to write 

char buf[1024] = { 0 };

instead


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

PS1, Line 728: OVERRIDE
nit: consider replacing with C++11-ish

Status GetExtentMap(ExtentMap* out) const override


PS1, Line 737: unique_ptr buf(new char[kBufSize]);
Is it crucial to allocate this on the heap?  Would stack allocated array 
suffice?  It could allow to get rid of memset() call as well:

char buf[kBufSize] = { 0 };

Or even

uint8_t buf[kBufSize] = { 0 };


PS1, Line 747: ~(0ULL)
Would using FIEMAP_MAX_OFFSET macro be more idiomatic?


PS1, Line 749:   int ret = ioctl(fd_, FS_IOC_FIEMAP, fm);
 :   if (ret == -1) {
nit: since the 'ret' variable is not referenced anywhere else, consider

if (ioctl(...) == -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: 1
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: Yes

[kudu-CR] Add ksck section to admin guide common workflows

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6598/1/docs/administration.adoc
File docs/administration.adoc:

PS1, Line 526: CLI command
"Command Line Interface command"? Probably just "CLI".


PS1, Line 552: processes
process


PS1, Line 577: --checksum-scan
We should decide whether to document --multi-word-flags or --multi_word_flags 
and apply that consistently. The rest of this file uses underscore-based flag 
names (because it was written before the gflags patch went in).


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

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

[kudu-CR] Add ksck section to admin guide common workflows

[Dan Burkert (Code Review)]

Hello Adar Dembo, Ambreen Kazi, Todd Lipcon,

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

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

to review the following change.

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

Add ksck section to admin guide common workflows

I've often wanted this when helping people through ksck.

Change-Id: I9631337b113d2c67be0057f728c68f792e8a4fd6
---
M docs/administration.adoc
1 file changed, 63 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9631337b113d2c67be0057f728c68f792e8a4fd6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: reduce contention on MemTracker::Release and Consume in block cache

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: WIP: reduce contention on MemTracker::Release and Consume in 
block cache
..

WIP: reduce contention on MemTracker::Release and Consume in block cache

This improved YCSB load throughput from ~450k ops/sec to ~570k.

Change-Id: Ic3bd24a452761b06611215f4831ef02238ba14bc
---
M src/kudu/util/cache.cc
1 file changed, 21 insertions(+), 2 deletions(-)


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

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

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 7: Code-Review+2

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

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

[kudu-CR] KUDU-579 [java client] Scanner fault tolerance

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-579 [java_client] Scanner fault tolerance
..


Patch Set 3:

(3 comments)

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

Line 377:   private static Integer findLeaderTabletServerPort(LocatedTablet 
tablet)
> Could this return int instead of Integer?
Done


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

PS3, Line 103: kill
> killing
Done


PS3, Line 112: restart
> restarting
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-579 [java client] Scanner fault tolerance

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-579 [java_client] Scanner fault tolerance
..

KUDU-579 [java_client] Scanner fault tolerance

This patch adds java client support to restart scanners if they
fail in the middle of table scanning. AsyncKuduScanner records
the final primary key retrieved in the previous scan. If a tserver
fails while scanning, the client marks the tserver as failed and
retries the scan elsewhere, providing its last primary key.

Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
---
M 
java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
A 
java/kudu-client/src/main/java/org/apache/kudu/client/ScannerExpiredException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
8 files changed, 233 insertions(+), 56 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: KUDU-1965: Allow user provided TLS certificates to work with 
KRPC
..

KUDU-1965: Allow user provided TLS certificates to work with KRPC

This patch adds a bool in the TlsContext class to keep track of
whether the certificates provided are externally provided or not.

If it is externally provided, then authenticated TLS is not
negotiated, causing a fallback to SASL authentication. The
certificates are used only for encryption.

Change-Id: Idd44770a1fe85e9934a657ee79d93139b9a86dff
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
4 files changed, 36 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd44770a1fe85e9934a657ee79d93139b9a86dff
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-579 [java client] Scanner fault tolerance

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-579 [java_client] Scanner fault tolerance
..


Patch Set 3:

(4 comments)

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

Line 78:* Ensures the scanner to be fault tolerant, and returns scan 
results in primary
> I think the C++ version reads a little bit better, could you adapt it?  The
Done


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

Line 245:   checkArgument(readMode == ReadMode.READ_AT_SNAPSHOT, "Returning 
rows in primary key order " +
> Update this message to be about fault tolerance instead of ordering.
Done


Line 474:   private final Callback 
gotNextRow =
> Why is this necessary?  Looks like it's never returning a delayed result.
Changed it because nextRowErrback is returning Deferred obj, so to be 
consistent thought should update here as well?


Line 506: if (isFaultTolerant && e instanceof ScannerExpiredException) {
> I think we can recover from more than just ScannerExpired; see https://gith
My understanding of how Java client error handling works for tablet server is 
inside TabletClient.dispatchTSErrorOrReturnException. There if encountered 
TABLET_NOT_FOUND and TABLET_NOT_RUNNING, it will retry. And for SCANNER_EXPIRED 
I changed it to throw ScannerExpiredException and then this error call back 
will be called and open a new scanner.  Not sure if my understanding is correct?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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-HasComments: No

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6574/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS6, Line 379: direct_data
> Does it make sense (if it possible at all) to add some test or function whi
it's hard to understand the indirect data by itself. The direct data has 
pointers places in it so we end up indirectly getting stuff form it (like in 
LOC 395/396), the indirect_data method is mostly so that the caller gets the 
location and size of it so that it can account for its memory or allocate a 
buffer of the same size.


PS6, Line 387: if (num_projected_cols != 4) return;
> Why is this shortcut?  What if there were just 3 projected columns?
the methods that call this only have one or all 4 columns projected. I changed 
this to return on the 1 case (clearer).


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

PS5, Line 120: //
> this should be
Done


PS5, Line 123: //
> this should be
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
..


Patch Set 1:

(3 comments)

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

PS1, Line 13: ecryptfs
> nit: eCryptfs
Done


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

PS1, Line 100: Status CheckFsFeatures(Env* env, const string& path) {
 :   // Arbitrary constants.
 :   static uint64_t kFileSize = 4096 * 4;
 :   static uint64_t kHoleOffset = 4096;
 :   static uint64_t kHoleSize = 8192;
 :   static uint64_t kPunchedFileSize = kFileSize - kHoleSize;
 : 
 :   // Open the test file.
 :   string filename = JoinPathSegments(path, 
"hole_punch_test_file");
 :   unique_ptr file;
 :   RWFileOptions opts;
 :   RETURN_NOT_OK(env->NewRWFile(opts, filename, ));
 : 
 :   // The file has been created; delete it on exit no matter what 
happens.
 :   ScopedFileDeleter file_deleter(env, filename);
 : 
 :   // Preallocate it, making sure the file's size is what we'd 
expect.
 :   uint64_t sz;
 :   RETURN_NOT_OK_PREPEND(file->PreAllocate(
 :   0, kFileSize, RWFile::CHANGE_FILE_SIZE), "could not 
preallocate file");
 :   RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, ));
 :   if (sz != kFileSize) {
 : return Status::IOError(Substitute(
 : "unexpected pre-punch file size for $0: expected $1 but 
got $2",
 : filename, kFileSize, sz));
 :   }
 : 
 :   // Punch the hole, testing the file's size again.
 :   RETURN_NOT_OK_PREPEND(file->PunchHole(kHoleOffset, kHoleSize),
 : "could not punch hole in file");
 :   RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, ));
 :   if (sz != kPunchedFileSize) {
 : return Status::IOError(Substitute(
 : "unexpected post-punch file size for $0: expected $1 but 
got $2",
 : filename, kPunchedFileSize, sz));
 :   }
 : 
 :   // Make sure we can get at the file's extents.
 :   RWFile::ExtentMap extents;
 :   RETURN_NOT_OK_PREPEND(file->GetExtentMap(),
 : "could not get file's extent map");
 :   return Status::OK();
 : }
> are there cases where people might have data already written to filesystems
That's a good question, and I don't fully know the answer. When I wrote the 
patch, I tested ext4 on el6.6 and on Ubuntu 16.04. Both supported hole punching 
as well as FS_IOC_FIEMAP.

Since you asked, I also tested ext4 and xfs on Ubuntu 14.04, SLES 12 and el6.4. 
Same result.

That's a pretty comprehensive list. What do you think?


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

Line 1306: Status LogBlockManager::Open(FsReport* report) {
> warning: default arguments on virtual or override methods are prohibited [g
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
Gerrit-PatchSet: 1
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: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
M src/kudu/rpc/rpc-test-base.h
6 files changed, 76 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6589/3/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

PS3, Line 129: level 
> level of
Done


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

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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
..

KUDU-1966: Data directories can be removed erroneously

Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 60 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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

[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC

[Sailesh Mukil (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1965: Allow user provided TLS certificates to work with 
KRPC
..

KUDU-1965: Allow user provided TLS certificates to work with KRPC

This patch adds a bool in the TlsContext class to keep track of
whether the certificates provided are externally provided or not.

If it is externally provided, then authenticated TLS is not
negotiated, causing a fallback to SASL authentication. The
certificates are used only for encryption.

Change-Id: Idd44770a1fe85e9934a657ee79d93139b9a86dff
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
4 files changed, 36 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd44770a1fe85e9934a657ee79d93139b9a86dff
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6574/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS6, Line 379: direct_data
Does it make sense (if it possible at all) to add some test or function which 
would exercise the KuduScanBatch::indirect_data method as well?


PS6, Line 387: if (num_projected_cols != 4) return;
Why is this shortcut?  What if there were just 3 projected columns?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1964. security: avoid calling ERR clear error() defensively

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1964. security: avoid calling ERR_clear_error() defensively
..


KUDU-1964. security: avoid calling ERR_clear_error() defensively

This changes our various code which wraps OpenSSL to avoid calling
ERR_clear_error() defensively. Instead, we now make sure to call
ERR_clear_error() after any case where we receive an error return value
from an OpenSSL library call.

For cases where we use the existing wrapper macros like
OPENSSL_RET_NOT_OK we are already ensured of this since the
GetOpenSSLErrors() call clears the error queue.

This provides a performance boost, since ERR_clear_error() ends up
taking several library-wide mutexes in OpenSSL 1.0.x (apparently
improved in OpenSSL 1.1, but that's not available on current OSes).

To ensure that we don't accidentally leave an OpenSSL error lying around
after any functions, I added a scoped helper which is active in debug
builds. This performs a DCHECK that the error queue is empty on scope
entry and exit.

To benchmark, I tested rpc-bench with encryption enabled using OpenSSL 1.0.1e
(RHEl6):

Master, SSL off:

I0404 12:49:04.811158  7250 rpc-bench.cc:84] Mode:Async
I0404 12:49:04.811172  7250 rpc-bench.cc:88] Client reactors:  16
I0404 12:49:04.811175  7250 rpc-bench.cc:89] Call concurrency: 60
I0404 12:49:04.811178  7250 rpc-bench.cc:92] Worker threads:   1
I0404 12:49:04.811182  7250 rpc-bench.cc:93] Server reactors:  4
I0404 12:49:04.811183  7250 rpc-bench.cc:94] --
I0404 12:49:04.811187  7250 rpc-bench.cc:95] Reqs/sec: 446998
I0404 12:49:04.811193  7250 rpc-bench.cc:96] User CPU per req: 9.97575us
I0404 12:49:04.811197  7250 rpc-bench.cc:97] Sys CPU per req:  12.2342us
I0404 12:49:04.811202  7250 rpc-bench.cc:98] Ctx Sw. per req:  0.604032

Master, SSL on:
--
I0404 12:57:10.241720  9949 rpc-bench.cc:86] Mode:Async
I0404 12:57:10.241736  9949 rpc-bench.cc:90] Client reactors:  16
I0404 12:57:10.241739  9949 rpc-bench.cc:91] Call concurrency: 60
I0404 12:57:10.241742  9949 rpc-bench.cc:94] Worker threads:   1
I0404 12:57:10.241745  9949 rpc-bench.cc:95] Server reactors:  4
I0404 12:57:10.241747  9949 rpc-bench.cc:96] Encryption:   1
I0404 12:57:10.241760  9949 rpc-bench.cc:97] --
I0404 12:57:10.241762  9949 rpc-bench.cc:98] Reqs/sec: 56761.3
I0404 12:57:10.241778  9949 rpc-bench.cc:99] User CPU per req: 39.7792us
I0404 12:57:10.241783  9949 rpc-bench.cc:100] Sys CPU per req:  106.916us
I0404 12:57:10.241786  9949 rpc-bench.cc:101] Ctx Sw. per req:  5.98631

Note the high number of context switches and system CPU. I gathered stack
traces of context switches using "perf record -g -e cs":

  Overhead   SamplesCommand  Shared Object 
Symbol
      .  .  
.

   100.00%180979  rpc-bench  [kernel.kallsyms]  [k] 
perf_event_task_sched_out
|
--- perf_event_task_sched_out
schedule
   |
   |--83.17%-- futex_wait_queue_me
   |  futex_wait
   |  do_futex
   |  sys_futex
   |  system_call_fastpath
   |  |
   |  |--83.11%-- __lll_lock_wait
   |  |  |
   |  |  |--42.33%-- int_thread_get
   |  |  |
   |  |  |--29.36%-- CRYPTO_add_lock
   |  |  |
   |  |  |--28.28%-- int_thread_get_item
   |  |   --0.03%-- [...]
   |  |
   |  |--16.89%-- pthread_cond_wait@@GLIBC_2.3.2
   |  |  |
   |  |  |--100.00%-- 
kudu::rpc::ServicePool::RunThread()
   |  |  |  
kudu::Thread::SuperviseThread(void*)
   |  |  |  start_thread
   |  |   --0.00%-- [...]
   |   --0.01%-- [...]
   |
   |--16.80%-- schedule_hrtimeout_range
   |  |
   |  |--100.00%-- ep_poll
   |  |  sys_epoll_wait
   |  |  system_call_fastpath
   |  |  epoll_wait
   |  |  |
   |  |   --100.00%-- ev_run
   |  | 
kudu::rpc::ReactorThread::RunThread()
   |  | 
kudu::Thread::SuperviseThread(void*)
   |  | start_thread
   |   

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 3:

(1 comment)

Much better.

http://gerrit.cloudera.org:8080/#/c/6589/3/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

PS3, Line 129: level 
level of


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

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

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
5 files changed, 76 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 5:

(2 comments)

Sorry for the confusion

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

PS5, Line 120: //
this should be

///


PS5, Line 123: ///
this should be

//

or just an empty string


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6574/4/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

PS4, Line 120: ///
> Leave this line empty of remove the 3rd slash.  Verbatim it should be:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
5 files changed, 76 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

PS1, Line 896: LOG(INFO)
> how big can this get? maybe VLOG?
I only get one extent on my laptop. I don't expect it to be big, and I also 
don't mind logging in tests if it helps debugging.


PS1, Line 908:   uint64_t found_offset = 0;
 :   for (const auto& e : extents) {
 : if (e.second >= (fs_block_size * 3)) {
 :   found_offset = e.first + fs_block_size;
 :   break;
 : }
 :   }
 :   if (found_offset == 0) {
 : LOG(INFO) << "Couldn't find extent to split, skipping this 
part of the test";
 : return;
 :   }
> how likely is this to happen? are we sure we won't be skipping this test mo
It's virtually impossible. One of the following would have to be true (assuming 
a 4096 fs block size):
1. In response to our 1k-at-a-time writes, the fs stupidly allocated one extent 
(sized to one block) every 4th write. Definitely won't happen with an ext4 that 
supports delayed allocation.
2. The filesystem was so fragmented that it couldn't even do a single 12k 
allocation.

But, I didn't want to assume anything about the test environment, so the code 
is forgiving.

On second thought, I'll eliminate case #1 by growing the file by preallocation 
instead of 1k-at-a-time writes.


PS1, Line 934: LOG(INFO)
> maybe VLOG?
Like I wrote above, I don't mind logging tests, and it can be helpful.


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

PS1, Line 587:   // If the underlying filesystem does not support extents, map 
entries
 :   // represent runs of adjacent fixed-size filesystem blocks 
instead.
> well in the apple case this does nothing, right? maybe elaborate a bit more
Sorry, this is actually intended to cover ext3.

I'll add another note for other platforms.


PS1, Line 589: typedef std::map ExtentMap;
> I like this way of typedeffing method arguments, maybe we should add this t
I'm not actually sure that it's the best approach for C++11. I think we should 
be doing:

  using ExtentMap = std::map;

But I didn't want to rock the boat too much.


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

PS1, Line 732: TRACE_EVENT1("io", "PosixRWFile::GetExtentMap", "path", 
filename_);
> why is this traceable? do we get extent maps online?
It's traceable because I believe the convention for all env_posix.cc functions 
is to trace if it involves any kind of IO (i.e. if it can block).


Line 740: struct fiemap* fm = (struct fiemap*)buf.get();
> warning: C-style casts are discouraged; use reinterpret_cast [google-readab
Done


PS1, Line 764: LOG(WARNING) << Substitute(
 :   "Unexpected extent found at offset $0", 
fme[i].fe_logical);
> log fatal/log error? is the data returned by this method still useful if th
I wasn't sure whether to trust that the kernel isn't buggy and crash if 
something unexpected happened, or whether to be more forgiving. But you're 
right that it's tough to reason about what to do with the data if the kernel 
gets it wrong.

I'll change this to be more strict.


PS1, Line 767: bool inserted = InsertIfNotPresent(,
 :fme[i].fe_logical,
 :fme[i].fe_length);
> we can get repeated extents? if not InsertOrDie?
Done


PS1, Line 771: // Two extents at the same logical offset? Shouldn't be possible.
 :   LOG(WARNING) << Substitute(
 :   "Multiple extents found at offset $0. Ignoring",
 :   fme[i].fe_logical);
> same comment as the case above
Done


PS1, Line 776: \n
> why the \n?
My mistake.


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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
..

KUDU-1966: Data directories can be removed erroneously

Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 60 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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

[kudu-CR] fs: generate report during Open

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 1:

(8 comments)

only skimmed through so far since I see David took a look

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

Line 7: fs: generate report during Open
do you have any measurement of whether this affects startup time on a system 
with lots of containers?


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

Line 65:it = missing_block_by_tablet_id.equal_range(it->first).second) {
Do we ever expect this to be a big map?

It seems like this loop might end up being klog(n) instead of k + log(n) since 
you are calling equal_range for every element that you hit?

The nested iterator with the same name as the outer iterator is also pretty 
confusing. From reading the code I can't quite tell if it's correct.

If we expect this to usually be small anyway, maybe it would be simpler to just 
use a map so we can use simpler foreach loops?


PS1, Line 68: bool first = true;
: for (auto it = range.first; it != range.second; it++) {
:   if (first) {
: first = false;
:   } else {
: block_str += ", ";
:   }
:   block_str += it->second;
: }
if you did the map> above, you could save a bunch of LOC here too 
and just use a JoinStrings() call


Line 78: s += Substitute("Fatal error: tablet $0 missing blocks: [ $1 ]\n",
can use SubstituteAndAppend


Line 228: s += Substitute("Misaligned block in container $0: $1\n",
SubstituteAndAppend


PS1, Line 324:   if (malformed_record_check && other.malformed_record_check) {
 : 
malformed_record_check->MergeFrom(other.malformed_record_check.get());
 :   } else if (other.malformed_record_check) {
 : malformed_record_check = other.malformed_record_check;
 :   }
hrm, this is very repetitive. you think it's worth doing a templated function 
or macro?


Line 405:   LOG(INFO) << ToString();
LOG has the downisde of a maximum 32kb (maybe 64kb?) limit, which I could see 
being annoying if you get a truncated report


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

Line 44: Entry(BlockId block_id, std::string tablet_id);
if this is just a simple assignment constructor, maybe it's not necessary and 
you could just use brace-initialization?


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

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6574/4/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

PS4, Line 120: ///
Leave this line empty of remove the 3rd slash.  Verbatim it should be:

/// @name Advanced/Unstable API
///
/// There are no guarantees on the stability of the format returned
/// by these methods, which might change at any given time.

///@{
... your new methods with their docs ...
///@}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 1:

(3 comments)

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

Line 338: } else if (r == 1) {
> warning: don't use else after return [readability-else-after-return]
Done


Line 372: if (full_containers_.size() == 0) {
> warning: the 'empty' method should be used to check for emptiness instead o
Done


Line 380:   if (all_containers_.size() == 0) {
> warning: the 'empty' method should be used to check for emptiness instead o
Done


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

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 1:

(22 comments)

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

Line 209:   virtual Status Open(FsReport* report = nullptr) = 0;
> warning: default arguments on virtual or override methods are prohibited [g
Done


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

Line 539: Status FileBlockManager::Open(FsReport* report) {
> warning: default arguments on virtual or override methods are prohibited [g
Done


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

Line 84:   Status Open(FsReport* report = nullptr) override;
> warning: default arguments on virtual or override methods are prohibited [g
Done


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

Line 83: using kudu::fs::CreateBlockOptions;
> warning: using decl 'CreateBlockOptions' is unused [misc-unused-using-decls
Done


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

Line 36: using std::vector;
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done


Line 212:   record(std::move(record)) {
> warning: passing result of std::move() as a const reference argument; no mo
Done


Line 236:   record(std::move(record)) {
> warning: passing result of std::move() as a const reference argument; no mo
Done


Line 263:   offset(std::move(offset)),
> warning: std::move of the variable of a trivially-copyable type has no effe
Done


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

PS1, Line 34:  
> prefix with: "Error type:"
Done


Line 45: 
> nit no need for an empty line, same for the structs below
Done


PS1, Line 56: deletes
> this doesn't delete anything right? maybe rephrase to: "(can be repaired by
I like the information on how a repairable error is repaired, but I'll reword 
this.


PS1, Line 77: container
> containers
Done


PS1, Line 77: preallocated
> pre-allocated
Disagree:

$ git grep -i pre-allocated | grep // | wc -l
5
$ git grep -i preallocated | grep // | wc -l
22


PS1, Line 80: truncates the container data files
> same comment as in the previous struct
Done


PS1, Line 101: Checks for LBM containers where one or both files are too short 
to be of use.
> care to elaborate on this? not sure I understand how files can be too short
Suppose we crash right after we've created a new data file but before we create 
the metadata file (or vice versa). Or suppose we get as far as creating both 
files but crash before writing the PBC container header into the metadata file. 
In either case, this is an "incomplete" container that contains no data. It's 
more annoying to reuse it than it's worth, so we just throw it out.

That's the kind of inconsistency this is targeting. I'll try to clarify.


PS1, Line 199: Fatal and repairable
> isn't a repairable inconsistency by definition non-fatal?
Unfortunately no.

The "partial record" inconsistency must be repaired in order for the LBM to 
function. If it isn't repaired, we won't be able to append new records to the 
metadata file. That's not so bad for CREATE records (we could get by with 
prohibiting new blocks in the container), but it means DELETEs can't be 
written, so existing blocks in the container can't be deleted.


PS1, Line 217:   // Returns whether this report describes at least one fatal 
and irreparable
 :   // inconsistency.
 :   bool HasFatalErrors() const;
> this method name and comment seems to agree with my comment on the struct h
Perhaps, but the implementation only looks for errors that are  
fatal+irreparable. That's what the comment says too.

If you think the method name isn't precise enough, to what would you have me 
change it?


PS1, Line 226:   // Like CheckForFatalErrors(), but also writes the report to 
LOG(INFO).
 :   Status LogAndCheckForFatalErrors() const;
 : 
 :   // Like CheckForFatalErrors(), but also writes the report to 
stdout.
 :   Status PrintAndCheckForFatalErrors() const;
> do there really belong here?, why not just pass a report all the time and t
They're just convenience methods since I've already repeated those patterns 
(four times for LogAndCheck and twice for PrintAndCheck).


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

Line 1304: Status LogBlockManager::Open(FsReport* report) {
> warning: default arguments on virtual or override methods are prohibited [g
Done


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

Line 

[kudu-CR] KUDU-1964. security: avoid calling ERR clear error() defensively

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1964. security: avoid calling ERR_clear_error() defensively
..


Patch Set 4: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6552/4/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:

Line 73:   SCOPED_OPENSSL_NO_PENDING_ERRORS;
good catch!


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

PS3, Line 67:   kudu::security::internal::ScopedCheckNoPendingSSLErrors 
_no_ssl_errors(__PRETTY_FUNCTION__)
> constructing the empty/unused object on the stack should already optimize i
ok, that makes sense


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b4421f4aae4d0e5a2d938881f9eea4e07ff2b10
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to release an rpc transfer's data

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 5:

(6 comments)

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

PS5, Line 10: obtain data for decoding
what do you mean by that?


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS5, Line 324: should
should be obtained? or MUST be obtained? (i.e after releasing transfer data, is 
it legal to call other methods on this instance?)


PS5, Line 323:   // Returns a pointer to the transfer data, including the 
header and all
 :   // sidecars. Pointers to individual sidecars should be 
obtained before
 :   // calling this method. That is, this method is not meant to 
return
 :   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
maybe move this doc to RpcController so it's user-facing, and just leave a 
breadcrumb here (like above)


PS5, Line 326:   // data for decoding but rather to provide a way to release 
ownership
 :   // of the whole transfer data.
 :  
if it only is used for ownership, then why do we need to get a Slice instead of 
just the pointer to the start? i.e we're only expecting the user to later 
'delete' this, right?


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

PS5, Line 192:   // Resets this controller, but releases the transfer data 
before.
 :   Status ReleaseTransferDataAndReset(Slice* release_data);
"transfer data" here is too implementation-centric.

I think the name should probably be like ReleaseResponseBufferAndReset or 
somesuch.


http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

PS5, Line 87:   // NOTE: buf_.release() doesn't memcpy if the transfer has 
completed, because
:   // buf_ must have been mandatorily resized from the initial 
capacity of 4 bytes
:   // to receive the whole message.
:  
not sure I follow. The initial capacity of faststring is 32 bytes, so it's 
still possible it as to alloc and memcpy, but we don't really care, because the 
case of a <32b response is rare?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6574/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 119:   /// Advanced/Unstable API
> It's possible to create an explicit doxygen member group for that.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
5 files changed, 76 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] fixed shortened TSK validity interval

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] fixed shortened TSK validity interval
..


[security] fixed shortened TSK validity interval

The TSK validity interval should be (authn_token_validity_interval +
tsk_propagation_interval + tsk_rotation_interval), which is
(auth_token_validity_interval + 2 * tsk_rotation_interval) since the
propagation interval is set equal to the rotation interval now.

Prior to this fix, as spotted by Dan, the TSK validity interval was
missing the rotation interval delta, which could lead to situations when
a valid authn token could not be verified due to already expired TSK.

Added an integration test to cover the fixed issue and exercise token
verification during and past token and its TSK lifecycle.

Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Reviewed-on: http://gerrit.cloudera.org:8080/6536
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
4 files changed, 190 insertions(+), 39 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
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] KUDU-1964. security: avoid calling ERR clear error() defensively

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: KUDU-1964. security: avoid calling ERR_clear_error() defensively
..

KUDU-1964. security: avoid calling ERR_clear_error() defensively

This changes our various code which wraps OpenSSL to avoid calling
ERR_clear_error() defensively. Instead, we now make sure to call
ERR_clear_error() after any case where we receive an error return value
from an OpenSSL library call.

For cases where we use the existing wrapper macros like
OPENSSL_RET_NOT_OK we are already ensured of this since the
GetOpenSSLErrors() call clears the error queue.

This provides a performance boost, since ERR_clear_error() ends up
taking several library-wide mutexes in OpenSSL 1.0.x (apparently
improved in OpenSSL 1.1, but that's not available on current OSes).

To ensure that we don't accidentally leave an OpenSSL error lying around
after any functions, I added a scoped helper which is active in debug
builds. This performs a DCHECK that the error queue is empty on scope
entry and exit.

To benchmark, I tested rpc-bench with encryption enabled using OpenSSL 1.0.1e
(RHEl6):

Master, SSL off:

I0404 12:49:04.811158  7250 rpc-bench.cc:84] Mode:Async
I0404 12:49:04.811172  7250 rpc-bench.cc:88] Client reactors:  16
I0404 12:49:04.811175  7250 rpc-bench.cc:89] Call concurrency: 60
I0404 12:49:04.811178  7250 rpc-bench.cc:92] Worker threads:   1
I0404 12:49:04.811182  7250 rpc-bench.cc:93] Server reactors:  4
I0404 12:49:04.811183  7250 rpc-bench.cc:94] --
I0404 12:49:04.811187  7250 rpc-bench.cc:95] Reqs/sec: 446998
I0404 12:49:04.811193  7250 rpc-bench.cc:96] User CPU per req: 9.97575us
I0404 12:49:04.811197  7250 rpc-bench.cc:97] Sys CPU per req:  12.2342us
I0404 12:49:04.811202  7250 rpc-bench.cc:98] Ctx Sw. per req:  0.604032

Master, SSL on:
--
I0404 12:57:10.241720  9949 rpc-bench.cc:86] Mode:Async
I0404 12:57:10.241736  9949 rpc-bench.cc:90] Client reactors:  16
I0404 12:57:10.241739  9949 rpc-bench.cc:91] Call concurrency: 60
I0404 12:57:10.241742  9949 rpc-bench.cc:94] Worker threads:   1
I0404 12:57:10.241745  9949 rpc-bench.cc:95] Server reactors:  4
I0404 12:57:10.241747  9949 rpc-bench.cc:96] Encryption:   1
I0404 12:57:10.241760  9949 rpc-bench.cc:97] --
I0404 12:57:10.241762  9949 rpc-bench.cc:98] Reqs/sec: 56761.3
I0404 12:57:10.241778  9949 rpc-bench.cc:99] User CPU per req: 39.7792us
I0404 12:57:10.241783  9949 rpc-bench.cc:100] Sys CPU per req:  106.916us
I0404 12:57:10.241786  9949 rpc-bench.cc:101] Ctx Sw. per req:  5.98631

Note the high number of context switches and system CPU. I gathered stack
traces of context switches using "perf record -g -e cs":

  Overhead   SamplesCommand  Shared Object 
Symbol
      .  .  
.

   100.00%180979  rpc-bench  [kernel.kallsyms]  [k] 
perf_event_task_sched_out
|
--- perf_event_task_sched_out
schedule
   |
   |--83.17%-- futex_wait_queue_me
   |  futex_wait
   |  do_futex
   |  sys_futex
   |  system_call_fastpath
   |  |
   |  |--83.11%-- __lll_lock_wait
   |  |  |
   |  |  |--42.33%-- int_thread_get
   |  |  |
   |  |  |--29.36%-- CRYPTO_add_lock
   |  |  |
   |  |  |--28.28%-- int_thread_get_item
   |  |   --0.03%-- [...]
   |  |
   |  |--16.89%-- pthread_cond_wait@@GLIBC_2.3.2
   |  |  |
   |  |  |--100.00%-- 
kudu::rpc::ServicePool::RunThread()
   |  |  |  
kudu::Thread::SuperviseThread(void*)
   |  |  |  start_thread
   |  |   --0.00%-- [...]
   |   --0.01%-- [...]
   |
   |--16.80%-- schedule_hrtimeout_range
   |  |
   |  |--100.00%-- ep_poll
   |  |  sys_epoll_wait
   |  |  system_call_fastpath
   |  |  epoll_wait
   |  |  |
   |  |   --100.00%-- ev_run
   |  | 
kudu::rpc::ReactorThread::RunThread()
   |  | 

[kudu-CR] Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Allow to release an rpc transfer's data
..

Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership and to allow for optimizations where no
memcpy's are done on the client side.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 72 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1964. security: avoid calling ERR clear error() defensively

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1964. security: avoid calling ERR_clear_error() defensively
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6552/3/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:

PS3, Line 114: Status::RuntimeError(
> Would it make sense to add GetOpenSSLErrors() here as well to clean up the 
Done


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

PS3, Line 67:   kudu::security::internal::ScopedCheckNoPendingSSLErrors 
_no_ssl_errors(__PRETTY_FUNCTION__)
> nit: would it make sense to put this under
constructing the empty/unused object on the stack should already optimize 
itself out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b4421f4aae4d0e5a2d938881f9eea4e07ff2b10
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
..


Patch Set 1:

(1 comment)

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

PS1, Line 100: Status CheckFsFeatures(Env* env, const string& path) {
 :   // Arbitrary constants.
 :   static uint64_t kFileSize = 4096 * 4;
 :   static uint64_t kHoleOffset = 4096;
 :   static uint64_t kHoleSize = 8192;
 :   static uint64_t kPunchedFileSize = kFileSize - kHoleSize;
 : 
 :   // Open the test file.
 :   string filename = JoinPathSegments(path, 
"hole_punch_test_file");
 :   unique_ptr file;
 :   RWFileOptions opts;
 :   RETURN_NOT_OK(env->NewRWFile(opts, filename, ));
 : 
 :   // The file has been created; delete it on exit no matter what 
happens.
 :   ScopedFileDeleter file_deleter(env, filename);
 : 
 :   // Preallocate it, making sure the file's size is what we'd 
expect.
 :   uint64_t sz;
 :   RETURN_NOT_OK_PREPEND(file->PreAllocate(
 :   0, kFileSize, RWFile::CHANGE_FILE_SIZE), "could not 
preallocate file");
 :   RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, ));
 :   if (sz != kFileSize) {
 : return Status::IOError(Substitute(
 : "unexpected pre-punch file size for $0: expected $1 but 
got $2",
 : filename, kFileSize, sz));
 :   }
 : 
 :   // Punch the hole, testing the file's size again.
 :   RETURN_NOT_OK_PREPEND(file->PunchHole(kHoleOffset, kHoleSize),
 : "could not punch hole in file");
 :   RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, ));
 :   if (sz != kPunchedFileSize) {
 : return Status::IOError(Substitute(
 : "unexpected post-punch file size for $0: expected $1 but 
got $2",
 : filename, kPunchedFileSize, sz));
 :   }
 : 
 :   // Make sure we can get at the file's extents.
 :   RWFile::ExtentMap extents;
 :   RETURN_NOT_OK_PREPEND(file->GetExtentMap(),
 : "could not get file's extent map");
 :   return Status::OK();
 : }
are there cases where people might have data already written to filesystems 
that don't support at least one of these things and which will become 
unreadable from now on?


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

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

[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
..


Patch Set 1: Code-Review+2

(1 comment)

feel free to ignore the nit or address it

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

PS1, Line 13: ecryptfs
nit: eCryptfs


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

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

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..

KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

This rejects unauthenticated connections from publicly routable IPs, even if
authentication and encryption are not configured. An unsafe flag
'allow_unauthenticated_public_connections' is provided to enable unauthenticated
connections from publicly routable IPs. Even though it is highly recommended to
disable it.

Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
---
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket.h
8 files changed, 138 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

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

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

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


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

PS1, Line 896: LOG(INFO)
how big can this get? maybe VLOG?


PS1, Line 908:   uint64_t found_offset = 0;
 :   for (const auto& e : extents) {
 : if (e.second >= (fs_block_size * 3)) {
 :   found_offset = e.first + fs_block_size;
 :   break;
 : }
 :   }
 :   if (found_offset == 0) {
 : LOG(INFO) << "Couldn't find extent to split, skipping this 
part of the test";
 : return;
 :   }
how likely is this to happen? are we sure we won't be skipping this test most 
of the time?


PS1, Line 934: LOG(INFO)
maybe VLOG?


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

PS1, Line 587:   // If the underlying filesystem does not support extents, map 
entries
 :   // represent runs of adjacent fixed-size filesystem blocks 
instead.
well in the apple case this does nothing, right? maybe elaborate a bit more?


PS1, Line 589: typedef std::map ExtentMap;
I like this way of typedeffing method arguments, maybe we should add this to 
the guidelines or something


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

PS1, Line 732: TRACE_EVENT1("io", "PosixRWFile::GetExtentMap", "path", 
filename_);
why is this traceable? do we get extent maps online?


PS1, Line 764: LOG(WARNING) << Substitute(
 :   "Unexpected extent found at offset $0", 
fme[i].fe_logical);
log fatal/log error? is the data returned by this method still useful if this 
happens?


PS1, Line 767: bool inserted = InsertIfNotPresent(,
 :fme[i].fe_logical,
 :fme[i].fe_length);
we can get repeated extents? if not InsertOrDie?


PS1, Line 771: // Two extents at the same logical offset? Shouldn't be possible.
 :   LOG(WARNING) << Substitute(
 :   "Multiple extents found at offset $0. Ignoring",
 :   fme[i].fe_logical);
same comment as the case above


PS1, Line 776: \n
why the \n?


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

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..

KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

This rejects unauthenticated connections from publicly routable IPs, even if
authentication and encryption are not configured. An unsafe flag
'allow_unauthenticated_public_connections' is provided to enable unauthenticated
connections from publicly routable IPs. Even though it is highly recommended to
disable it.

Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
---
A src/kudu-1875.patch
A src/kudu-1875patch
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket.h
10 files changed, 932 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6514/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:

Line 145:   if (!FLAGS_allow_unauthenticated_public_connections &&
> Instead of linking to the blog, it's probably best to reference the associa
Done


Line 149: Status s = RejectUntrustedPublicConnection(addr);
> Hmm, good point.  We really only need to be checking when SASL PLAIN is bei
Done


Line 685:   // verified.
> argh, good point.  I forgot that negotiated_mech_ isn't set till later.  Th
Done


http://gerrit.cloudera.org:8080/#/c/6514/1/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:

PS1, Line 117: 0xff;
> could this just be 0xff?  Having an odd # of hex digits is somewhat unordin
Done


Line 121: return true;
> The google style guide allows omitting the braces here, but I think our usu
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 3:

(1 comment)

Just a tiny nit to improve doxygen-related documentation for the new methods.

http://gerrit.cloudera.org:8080/#/c/6574/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 119:   /// Advanced/Unstable API
It's possible to create an explicit doxygen member group for that.

Just put these two new methods inside an construct which looks like the 
following:

/// @name Advanced/Unstable API
/// 
///@{

... your new methods with their docs ...

///@}

For details please see http://www.doxygen.nl/grouping.html#memgroup ; for an 
example of rendered documentation (Group2): 
http://www.doxygen.nl/examples/memgrp/html/class_test.html

There an example in src/kudu/common/partial_row.h


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [python] Fix flaky test test connect timeouts

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [python] Fix flaky test test_connect_timeouts
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] [python] Fix flaky test test connect timeouts

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: [python] Fix flaky test test_connect_timeouts
..


[python] Fix flaky test test_connect_timeouts

It only tests passing the timeout, but it's low enough that on our poor
jenkins workers it might actually time out connecting to the cluster.

Change-Id: I9df059724ae77c404bccc594e4fa01ac75d905a5
Reviewed-on: http://gerrit.cloudera.org:8080/6573
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M python/kudu/tests/test_client.py
1 file changed, 2 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9df059724ae77c404bccc594e4fa01ac75d905a5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] fixed shortened TSK validity interval

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] fs: generate report during Open

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 1:

(11 comments)

first round of reviews. this patch is pretty heftly so I'll review it in 2-3 
rounds or less. rather do that than leave no feedback until I'm done

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

PS1, Line 32:  previously only the opposite was true.
what is the opposite?


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

PS1, Line 34:  
prefix with: "Error type:"
same for the cases below


Line 45: 
nit no need for an empty line, same for the structs below


PS1, Line 56: deletes
this doesn't delete anything right? maybe rephrase to: "(can be repaired by 
deleting the block) though I'd be okay to just omit this altogether


PS1, Line 77: preallocated
pre-allocated


PS1, Line 77: container
containers


PS1, Line 80: truncates the container data files
same comment as in the previous struct


PS1, Line 101: Checks for LBM containers where one or both files are too short 
to be of use.
care to elaborate on this? not sure I understand how files can be too short. 
were they truncated? (in which case how is this repairable?)


PS1, Line 199: Fatal and repairable
isn't a repairable inconsistency by definition non-fatal?
how about just:
- fatal
- repairable
- irreparable


PS1, Line 217:   // Returns whether this report describes at least one fatal 
and irreparable
 :   // inconsistency.
 :   bool HasFatalErrors() const;
this method name and comment seems to agree with my comment on the struct header


PS1, Line 226:   // Like CheckForFatalErrors(), but also writes the report to 
LOG(INFO).
 :   Status LogAndCheckForFatalErrors() const;
 : 
 :   // Like CheckForFatalErrors(), but also writes the report to 
stdout.
 :   Status PrintAndCheckForFatalErrors() const;
do there really belong here?, why not just pass a report all the time and then 
let the called decide what to do with it? (print it or log it or whatever)


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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 2:

(1 comment)

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

Line 164:   // Check that all of the expected uuids are present.
> There's something that still seems unintuitive about this check to me. Mayb
The way I've structured it is the least invasive in terms of code changes and 
runtime overhead.  If I were going to do something like that I'd be more 
inclined to restructure the method pretty significantly.  Given the poor error 
messages, I think I'll just go ahead with that.


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

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

[kudu-CR] KUDU-579 [java client] Scanner fault tolerance

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-579 [java_client] Scanner fault tolerance
..


Patch Set 3:

(10 comments)

I think overall the error handling needs some work; it looks like right now 
it's only handling scanner expired and not necessarily anything else (if I 
understand correctly).  Also the change to the master location that JD brought 
up, not sure what's going on there, but I agree that the change doesn't seem 
correct.

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

PS3, Line 11: scan
batch


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

Line 78:* Ensures the scanner to be fault tolerant, and returns scan 
results in primary
I think the C++ version reads a little bit better, could you adapt it?  The 
note about hash partitioning can be removed.  
https://github.com/apache/kudu/blob/master/src/kudu/client/client.h#L1969-L1981


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

Line 245:   checkArgument(readMode == ReadMode.READ_AT_SNAPSHOT, "Returning 
rows in primary key order " +
Update this message to be about fault tolerance instead of ordering.


Line 474:   private final Callback 
gotNextRow =
Why is this necessary?  Looks like it's never returning a delayed result.


Line 506: if (isFaultTolerant && e instanceof ScannerExpiredException) {
I think we can recover from more than just ScannerExpired; see 
https://github.com/apache/kudu/blob/master/src/kudu/client/scanner-internal.cc#L66


Line 511:   return Deferred.fromError(e); // Let the error propagates.
'propagate' was more correct.


Line 835:   Status statusIncomplete = Status.Incomplete("Cannot 
continue scanning, " +
Is this case being handled?


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

Line 377:   private static Integer findLeaderTabletServerPort(LocatedTablet 
tablet)
Could this return int instead of Integer?


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

PS3, Line 103: kill
killing


PS3, Line 112: restart
restarting


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [consensus] fixed typos in consensus flags tagging

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [consensus] fixed typos in consensus flags tagging
..


[consensus] fixed typos in consensus flags tagging

Change-Id: I39fb954313368c8f612c986ab13dd47e8c948998
Reviewed-on: http://gerrit.cloudera.org:8080/6580
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/consensus_peers.cc
1 file changed, 6 insertions(+), 5 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I39fb954313368c8f612c986ab13dd47e8c948998
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [consensus] fixed typos in consensus flags tagging

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [consensus] fixed typos in consensus flags tagging
..


Patch Set 1:

(1 comment)

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

Line 54: TAG_FLAG(raft_get_node_instance_timeout_ms, hidden);
> honestly I forget the intent (if I ever even knew it) but IMO it makes sens
OK, thanks!  Then this fix makes sense.


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

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

[kudu-CR] [consensus] fixed typos in consensus flags tagging

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [consensus] fixed typos in consensus flags tagging
..


Patch Set 1:

(1 comment)

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

Line 54: TAG_FLAG(raft_get_node_instance_timeout_ms, hidden);
> I meant line 50 makes it consensus_rpc_timeout advanced.  If they wanted to
honestly I forget the intent (if I ever even knew it) but IMO it makes sense to 
have consensus_rpc_timeout_ms as advanced and not hidden as this more likely to 
be tweaked than raft_get_node_instance_timeout_ms


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

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

[kudu-CR] WIP: Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Allow to release an rpc transfer's data
..

WIP: Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership to allow for optimizations where no
memcpy's are done on the client side.

WIP: getting some jenkins mileage

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 72 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 2:

(1 comment)

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

Line 164:   // Check that all of the expected uuids are present.
There's something that still seems unintuitive about this check to me. Maybe 
it's because it's inside the loop (which is weird because this should only 
happen once), or because it's comparing container sizes instead of contents.

What if, outside the loop, we compared first_all_uuids.first with 
JoinStrings(uuids.keys, ",")? I think I'd find that more intuitive; do you as 
well?


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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: Add --redact=none flag option for specifying no redaction
..


Add --redact=none flag option for specifying no redaction

It's still possible to specify no redaction via --redact='', but I was
only able to determine how to do that by reading the code.  I think this
makes it a bit easier to use.

Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
Reviewed-on: http://gerrit.cloudera.org:8080/6590
Reviewed-by: Adar Dembo 
Reviewed-by: Hao Hao 
Tested-by: Kudu Jenkins
---
M src/kudu/util/flags.cc
1 file changed, 26 insertions(+), 38 deletions(-)

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



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

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

[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

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

Change subject: KUDU-1965: Allow user provided TLS certificates to work with 
KRPC
..

KUDU-1965: Allow user provided TLS certificates to work with KRPC

This patch adds a bool in the TlsContext class to keep track of
whether the certificates provided are externally provided or not.

If it is externally provided, then authenticated TLS is not
negotiated, causing a fallback to SASL authentication. The
certificates are used only for encryption.

Change-Id: Idd44770a1fe85e9934a657ee79d93139b9a86dff
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
4 files changed, 34 insertions(+), 17 deletions(-)


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

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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 1:

(1 comment)

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

Line 164:   // Check that all of the expected paths are present.
> I can buy that, but the errors (and comments) should at least be consistent
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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: Yes

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 4: Code-Review+1

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

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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
..

KUDU-1966: Data directories can be removed erroneously

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 14 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 4:

The differences is that the flags are now getting reset back to the default 
value every time the validator runs.  During tests, the validator would get run 
twice: once with the default 'all' value, then again with the 'flag' value 
specified in test_util.cc.

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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 4: Code-Review+2

I'm curious as to how the diff between PS4 and PS3 fixed the test failures. As 
far as I can tell, all that changed is the location of the default 
initialization of the global booleans. Why did the first version break the 
tests, and how does this change fix them?

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

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

[kudu-CR] [docs] Add security guide

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [docs] Add security guide
..


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6479/4/docs/security.adoc
File docs/security.adoc:

PS4, Line 31: will explain
> describes
Done


PS4, Line 66: requiring
: certificates be manually deployed on every node.
> requiring you to manually deploy certificates on every node.
Done


PS4, Line 168: turned off configuring the
 : `--redact` flag
> by setting --redact to false?
No, it's unfortunately not quite as simple as that.  I think we're still 
actively working on the redact flag, I'm not sure exactly how to document it at 
this point.


PS4, Line 220: yet
> Remove 'yet'.
Done


PS4, Line 226: yet 
> Remove.
Done


PS4, Line 229:  yet
> Remove'.
Done


PS4, Line 242: The 
> Remove 'the'
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iabf60804975dc105243626be48d3a141c9a4dab5
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [docs] Add security guide

[Dan Burkert (Code Review)]

Hello Hao Hao, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: [docs] Add security guide
..

[docs] Add security guide

Change-Id: Iabf60804975dc105243626be48d3a141c9a4dab5
---
A docs/security.adoc
1 file changed, 243 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabf60804975dc105243626be48d3a141c9a4dab5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add --redact=none flag option for specifying no redaction
..

Add --redact=none flag option for specifying no redaction

It's still possible to specify no redaction via --redact='', but I was
only able to determine how to do that by reading the code.  I think this
makes it a bit easier to use.

Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
---
M src/kudu/util/flags.cc
1 file changed, 26 insertions(+), 38 deletions(-)


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

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

[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has abandoned this change.

Change subject: KUDU-1965: Allow user provided TLS certificates to work with 
KRPC
..


Abandoned

Patch is incorrect. Will post another one shortly.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Allow to release an rpc transfer's data
..

WIP: Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership to allow for optimizations where no
memcpy's are done on the client side.

WIP: getting some jenkins mileage

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 72 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 3:

The change looks good to me, but can you check why the tests are failing? It 
looks like related.

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

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

[kudu-CR] [security-faults-itest] fix test flakiness

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security-faults-itest] fix test flakiness
..


[security-faults-itest] fix test flakiness

When running the security-faults-itest on a slow VM with other
activities in parallel, it might take up to 8 seconds to start a master.
So, bumping the ticket TTL to higher value so Kudu cluster with 3
masters finishes starting up when initially acquired tickets are valid.

Also, updated Raft parameters interval to speed up leader election.

Change-Id: I132426ee37358a228cf3f5d1fef9e9ec08610b16
Reviewed-on: http://gerrit.cloudera.org:8080/6586
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/integration-tests/security-faults-itest.cc
1 file changed, 11 insertions(+), 4 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I132426ee37358a228cf3f5d1fef9e9ec08610b16
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: WIP: Allow to release an rpc transfer's data
..

WIP: Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership to allow for optimizations where no
memcpy's are done on the client side.

WIP: getting some jenkins mileage

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 72 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] whole batch release, but wrong

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has abandoned this change.

Change subject: whole batch release, but wrong
..


Abandoned

oops forgot to squash

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib2080a9832b096b13a1a5b9ce88ef522e30b0e00
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] whole batch release, but wrong

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: whole batch release, but wrong
..

whole batch release, but wrong

Change-Id: Ib2080a9832b096b13a1a5b9ce88ef522e30b0e00
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
5 files changed, 36 insertions(+), 0 deletions(-)


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

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

[kudu-CR] WIP: Allow to release an rpc transfer's data

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: WIP: Allow to release an rpc transfer's data
..

WIP: Allow to release an rpc transfer's data

This adds a way to release a transfer's data to a caller.
This is not meant to obtain data for decoding, but rather
to release ownership to allow for optimizations where no
memcpy's are done on the client side.

Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
WIP: getting some jenkins mileage
---
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/transfer.h
7 files changed, 46 insertions(+), 11 deletions(-)


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

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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 1:

(1 comment)

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

Line 164:   // Check that all of the expected paths are present.
> My thought was that all of the error messages in this method are unintuitiv
I can buy that, but the errors (and comments) should at least be consistent 
with one another, and right now this one stands out from the others.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6574/1/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 124:  private:
> Missed this?
Done


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

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

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new patch set (#3).

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
5 files changed, 79 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 1:

(1 comment)

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

Line 164:   // Check that all of the expected paths are present.
> It's a little unintuitive to talk about paths here and not uuids; maybe "Ch
My thought was that all of the error messages in this method are unintuitive, 
since the user is specifying data directories, and the error messages are 
talking about UUIDs, which is entirely an internal detail.  I think UUIDs are 
even less clear than paths in the error message, so I'm a little bit reluctant 
to change it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
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: Yes

[kudu-CR] [security-faults-itest] fix test flakiness

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [security-faults-itest] fix test flakiness
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6574/1/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 124:   ///
> Nit: separate from indirect_data() with an empty line.
Missed this?


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

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

[kudu-CR] KUDU-1875: Refuse unauthenticated connections from publicly routable IP addrs

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1875: Refuse unauthenticated connections from publicly 
routable IP addrs
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6514/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:

Line 149:   negotiated_authn_ == AuthenticationType::INVALID)) {
> Yeah, initially I thought the same. But it turns out here negotiated_mech_ 
Hmm, good point.  We really only need to be checking when SASL PLAIN is being 
used, so perhaps the check is only necessary below, and not here?  You are 
right that we don't know whether we're using SASL GSSAPI or SASL PLAIN until 
below.


Line 685:   if (!FLAGS_allow_unauthenticated_public_connections &&
> I am checking here because negotiated_mech_ is only set properly at line 67
argh, good point.  I forgot that negotiated_mech_ isn't set till later.  The 
mech should still not be INVALID here, though.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c3fbb5491785874c5701d6c9d866949cfac905e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Harsh J 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: Allow to get the raw data from a KuduScanBatch
..

Allow to get the raw data from a KuduScanBatch

This allows to fetch both the direct and the indirect raw data from
a KuduScanBatch. Exposing this opens the door for Impala to do
whole batch memcpy, instead of row by row. Ideally there would be
no memcpying at all, followup patches will allow for that.

Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.h
M src/kudu/common/schema.h
5 files changed, 78 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to get the raw data from a KuduScanBatch

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Allow to get the raw data from a KuduScanBatch
..


Patch Set 1:

ok, makes sense. will do that in a follow up patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c9ad5aa7c5f45a87827352597a404241912342f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..


Patch Set 1:

(1 comment)

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

Line 164:   // Check that all of the expected paths are present.
It's a little unintuitive to talk about paths here and not uuids; maybe "Check 
that the number of instance files is the same as the number of UUIDs"?


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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add --redact=none flag option for specifying no redaction
..

Add --redact=none flag option for specifying no redaction

It's still possible to specify no redaction via --redact='', but I was
only able to determine how to do that by reading the code.  I think this
makes it a bit easier to use.

Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
---
M src/kudu/util/flags.cc
M src/kudu/util/logging.cc
2 files changed, 26 insertions(+), 41 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Add --redact=none flag option for specifying no redaction
..


Patch Set 1:

(2 comments)

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

Line 136:   } else if (redact_flags == "NONE" || redact_flags.empty()) {
> warning: don't use else after return [readability-else-after-return]
Done


Line 140:   for (const auto& t : strings::Split(redact_flags, ",", 
strings::SkipEmpty())) {
> warning: the variable '__end' is copy-constructed from a const reference bu
no idea what this means


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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Dan Burkert has uploaded a new patch set (#2).

Change subject: Add --redact=none flag option for specifying no redaction
..

Add --redact=none flag option for specifying no redaction

It's still possible to specify no redaction via --redact='', but I was
only able to determine how to do that by reading the code.  I think this
makes it a bit easier to use.

Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
---
M src/kudu/util/flags.cc
M src/kudu/util/logging.cc
2 files changed, 21 insertions(+), 41 deletions(-)


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

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

[kudu-CR] Add --redact=none flag option for specifying no redaction

[Dan Burkert (Code Review)]

Hello Hao Hao, Todd Lipcon,

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

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

to review the following change.

Change subject: Add --redact=none flag option for specifying no redaction
..

Add --redact=none flag option for specifying no redaction

It's still possible to specify no redaction via --redact='', but I was
only able to determine how to do that by reading the code.  I think this
makes it a bit easier to use.

Change-Id: I06f9d1ab08baaac02359e3ab29f8741bd1f4ff7d
---
M src/kudu/util/flags.cc
1 file changed, 18 insertions(+), 39 deletions(-)


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

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

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

[Dan Burkert (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: KUDU-1966: Data directories can be removed erroneously
..

KUDU-1966: Data directories can be removed erroneously

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 14 insertions(+), 0 deletions(-)


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

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

[kudu-CR] KUDU-1424. Add getters to PartialRow

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: KUDU-1424. Add getters to PartialRow
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6554/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java:

Line 149:* @return A boolean
> nit: here and everywhere below, start with a lower case.
Done


http://gerrit.cloudera.org:8080/#/c/6554/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java:

Line 82: // This is the expected exception
> nit: Here and elsewhere, inline comments should end with a period.
Done


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

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