[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess
..

KUDU-2208 Add RETRY_ON_EINTR() to Subprocess

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Since the pthread_kill() signal is sent
asynchronously, sometimes the pthread_kill() raises error signal 3.
In that condition, the unit test logs the incident as an INFO.

Prior to this bug fix patch, the Subprocess throws an exception
because it cannot handle the kill signal in Wait() state. To fix the bug,
we add RETRY_ON_EINTR() inside Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 73 insertions(+), 22 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 9
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [rpc] don't issue authn tokens over non-confidential connections

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9025


Change subject: WIP [rpc] don't issue authn tokens over non-confidential 
connections
..

WIP [rpc] don't issue authn tokens over non-confidential connections

With this patch, master will not issue authn tokens over a
non-confidential connection.  The 'confidentiality' property is set
while negotiating a connection and stored into the RpcContext
(accessible via the RpcContext::is_confidential() method).

WIP: a test is needed. Also, I would like to collect some feedback
 on the way how the 'confidentiality' property is determined
 and propagated.

Change-Id: Ie31aa492bcc460dbd43975bccfe571354f3bf885
---
M src/kudu/master/master_service.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
6 files changed, 37 insertions(+), 2 deletions(-)



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

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

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..

KUDU-2208 Add unit test to detect Subprocess Interruption
Handling failure and patch to fix the bug

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Since the pthread_kill() signal is sent
asynchronously, sometimes the pthread_kill() raises error signal 3.
In that condition, the unit test logs the incident as an INFO.

Prior to this bug fix patch, the Subprocess throws an exception
because it cannot handle the kill signal in Wait() state. To fix the bug,
we add RETRY_ON_EINTR() inside Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 69 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 8
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8978 )

Change subject: KUDU-2202: support for removing data directories (take two)
..


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8978/4/src/kudu/fs/data_dirs.cc@810
PS4, Line 810: list of data directories now
 : // includes the missing directory.
nit: If I'm understanding this correctly, the update should have removed the 
UUID, so maybe replace "includes" with "excludes"?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 13 Jan 2018 03:42:45 +
Gerrit-HasComments: Yes

[kudu-CR] fs: defer failure from metadata load to bootstrap when data dir is missing

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8376 )

Change subject: fs: defer failure from metadata load to bootstrap when data dir 
is missing
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e8d5697c2bb08287cce11fbdab6fb8d6e37d1ad
Gerrit-Change-Number: 8376
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 13 Jan 2018 03:28:38 +
Gerrit-HasComments: No

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8989 )

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..

[raft_consensus-itest] proper fix for Test_KUDU_1735

Adapted the logic of the Test_KUDU_1735 scenario to work for both
3-2-3 and 3-4-3 replication schemes.

This is a follow-up for 8bcc80eec075321faef26b2a0ccac12ac9d7.

Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Reviewed-on: http://gerrit.cloudera.org:8080/8989
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/util/fault_injection.cc
2 files changed, 91 insertions(+), 25 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8989 )

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Sat, 13 Jan 2018 03:18:48 +
Gerrit-HasComments: No

[kudu-CR] tools: Add debug mode to pb dump tool

[Mike Percy (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.


Change subject: tools: Add debug mode to pb dump tool
..

tools: Add debug mode to pb dump tool

This is useful when dumping a partially-flushed file to determine the
start of the damaged entry. To remove the damaged entry, the file can be
truncated at the beginning of the offending entry. See KUDU-2260.

TODO: Should this be called "verbose" mode instead of debug mode?

Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80
---
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
3 files changed, 36 insertions(+), 2 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80
Gerrit-Change-Number: 9024
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..

KUDU-2208 Add unit test to detect Subprocess Interruption
Handling failure and patch to fix the bug

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Since the pthread_kill() signal is sent
asynchronously, sometimes the pthread_kill() raises error signal 3.
In that condition, the unit test logs the incident as an INFO.

Prior to this bug fix patch, the Subprocess throws an exception
because it cannot handle the kill signal in Wait() state. To fix the bug,
we add RETRY_ON_EINTR() inside Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 65 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 7
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8982 )

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..


Patch Set 6: Code-Review+2

Carrying over Todd's +2 (just an IWYU change)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 13 Jan 2018 00:59:59 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8982 )

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..

KUDU-2253 Deltafile on-disk size is 3x larger than expected

While looking into the performance of the integration test written for
KUDU-2251 (https://gerrit.cloudera.org/#/c/8951/ revision 6), Todd and I
found that the on-disk deltafiles written are about 3x larger than
expected. The culprit is an optimization in the CFile value index which
is turned off for delta files. The optimization truncates large keys
after the first unique byte between sequential values. The deltafile
values, in the case of this integration test, include the small
DeltaKey, and the 8KiB updated value. As a result the BTree interior
nodes are being completely filled by only ~4 values (32KiB cblock size
by default). This makes the BTree far less effective, and means that the
full updated data is written many times. We expect fixing this will
improve performance for update-heavy workloads with large values (for
example, YCSB).

Unfortunately, fixing the issue is not quite as simple as enabling the
optimization for deltafiles, since in the normal course of seeking
through deltafiles during a scan, we deserialze the value index keys
into a DeltaKey. If the values are truncated this deserialization step
can fail. Instead, this patch adds overridable value index key encoding
to CFileWriter, and delta file overrides it to only encode the delta
key, which is a pair of variable-length integers.

Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Reviewed-on: http://gerrit.cloudera.org:8080/8982
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
8 files changed, 73 insertions(+), 36 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

[Adar Dembo (Code Review)]

Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
..


Patch Set 1: Verified+1

One ASAN test timed out in table creation, which I think is unrelated to this 
commit.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 13 Jan 2018 00:58:49 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

[Adar Dembo (Code Review)]

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.


Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
..

KUDU-1913: LIFO wake up ordering for threadpool worker threads

This patch changes the wake-up ordering for idle pool worker threads
to be LIFO. Previously all worker threads idled on a single
ConditionVariable which, by virtue of using pthread_cond_t under the hood,
was FIFO ordered.

In the abstract, FIFO ordering ensures a fair distribution of work amongst a
set of threads, but that's totally undesirable in a thread pool where the
goal is to get work done as quickly and with as few threads as possible. For
example, a fast stream of cheap tasks (e.g. RPCs) should be serviceable via
a single thread or something close to that.

The new unit test was looped 1000 times in TSAN mode with 8 stress threads
to shake out any flakes. Additionally, I ran through a variation of Todd's
test from a past code review [1]. I spun up three tservers, two of which had
failure detection disabled so that the third would get all of the leader
replicas. I created 240 tablets and looked at the size of the Raft thread
pool on that tserver. The steady state thread count was mildly lower at
first (4 vs. 8) but was much lower (4 vs. 30) after a SIGSTOP+SIGCONT cycle.

1. https://gerrit.cloudera.org/c/7331

Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
---
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
5 files changed, 114 insertions(+), 15 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Jeffrey F. Lukman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9015 )

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h
File src/kudu/util/os-util.h:

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h@33
PS5, Line 33: #define RETRY_ON_EINTR(err, expr) do { \
> need to #include  in this header so that we get access to std:
Done


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc
File src/kudu/util/subprocess-test.cc:

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@28
PS5, Line 28: #include 
> per Alexey's comment please separate out the C includes from the C++ ones h
Done


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@312
PS5, Line 312: t_started = true;
> still think we should add a short sleep here so that we ensure that the sig
Done


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@313
PS5, Line 313: SleepFor(MonoDelta::FromMilliseconds(50));
> nit: we have to use CHECK_OK here because gtest isn't thread-safe to run as
Done


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@332
PS5, Line 332:   ASSERT_TRUE(pthread_kill(t, SIGUSR2) == 0);
> but the "user defined signal 2 error" is the bug we're trying to provoke he
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 6
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 23:51:13 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..

KUDU-2208 Add unit test to detect Subprocess Interruption
Handling failure and patch to fix the bug

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Prior to the bug fix in this patch,
the Subprocess throws an exception because it cannot handle
the kill signal in Wait() state. The bug fix adds RETRY_ON_EINTR()
at Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 60 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 6
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8989 )

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2428
PS3, Line 2428:   vector tservers;
> nit: this vector isn't really needed anymore
Done


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2430
PS3, Line 2430:   ASSERT_GE(tservers.size(), 3);
> nit: I don't think this is really required but if so it can just be ASSERT_
Done


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2432
PS3, Line 2432: tservers[0]
> nit: how about:
Good idea!


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2477
PS3, Line 2477: attemp
> typo: attempt
Done


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2497
PS3, Line 2497:   ASSERT_EVENTUALLY([&] {
> nit: this ASSERT_EVENTUALLY can go outside the loop
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 12 Jan 2018 23:21:42 +
Gerrit-HasComments: Yes

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Alexey Serbin (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..

[raft_consensus-itest] proper fix for Test_KUDU_1735

Adapted the logic of the Test_KUDU_1735 scenario to work for both
3-2-3 and 3-4-3 replication schemes.

This is a follow-up for 8bcc80eec075321faef26b2a0ccac12ac9d7.

Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
---
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/util/fault_injection.cc
2 files changed, 91 insertions(+), 25 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..

KUDU-2253 Deltafile on-disk size is 3x larger than expected

While looking into the performance of the integration test written for
KUDU-2251 (https://gerrit.cloudera.org/#/c/8951/ revision 6), Todd and I
found that the on-disk deltafiles written are about 3x larger than
expected. The culprit is an optimization in the CFile value index which
is turned off for delta files. The optimization truncates large keys
after the first unique byte between sequential values. The deltafile
values, in the case of this integration test, include the small
DeltaKey, and the 8KiB updated value. As a result the BTree interior
nodes are being completely filled by only ~4 values (32KiB cblock size
by default). This makes the BTree far less effective, and means that the
full updated data is written many times. We expect fixing this will
improve performance for update-heavy workloads with large values (for
example, YCSB).

Unfortunately, fixing the issue is not quite as simple as enabling the
optimization for deltafiles, since in the normal course of seeking
through deltafiles during a scan, we deserialze the value index keys
into a DeltaKey. If the values are truncated this deserialization step
can fail. Instead, this patch adds overridable value index key encoding
to CFileWriter, and delta file overrides it to only encode the delta
key, which is a pair of variable-length integers.

Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
8 files changed, 73 insertions(+), 36 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8982 )

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 12 Jan 2018 22:11:14 +
Gerrit-HasComments: No

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8989 )

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..


Patch Set 3:

(5 comments)

looks good, just some small nits

http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2428
PS3, Line 2428:   vector tservers;
nit: this vector isn't really needed anymore


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2430
PS3, Line 2430:   ASSERT_GE(tservers.size(), 3);
nit: I don't think this is really required but if so it can just be ASSERT_EQ(3


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2432
PS3, Line 2432: tservers[0]
nit: how about:

  tablet_servers_[cluster_->tablet_server(0)->uuid()]

here and on the line below?


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2477
PS3, Line 2477: attemp
typo: attempt


http://gerrit.cloudera.org:8080/#/c/8989/3/src/kudu/integration-tests/raft_consensus-itest.cc@2497
PS3, Line 2497:   ASSERT_EVENTUALLY([&] {
nit: this ASSERT_EVENTUALLY can go outside the loop



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 12 Jan 2018 21:53:12 +
Gerrit-HasComments: Yes

[kudu-CR] [docs] Removed the note about KUDU-1626

[Alex Rodoni (Code Review)]

Alex Rodoni has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9020


Change subject: [docs] Removed the note about KUDU-1626
..

[docs] Removed the note about KUDU-1626

Change-Id: Ia7b5e127cd9d1e2bda60a20de62fafd95e5f93e3
---
M docs/schema_design.adoc
1 file changed, 0 insertions(+), 5 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7b5e127cd9d1e2bda60a20de62fafd95e5f93e3
Gerrit-Change-Number: 9020
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..

KUDU-2253 Deltafile on-disk size is 3x larger than expected

While looking into the performance of the integration test written for
KUDU-2251 (https://gerrit.cloudera.org/#/c/8951/ revision 6), Todd and I
found that the on-disk deltafiles written are about 3x larger than
expected. The culprit is an optimization in the CFile value index which
is turned off for delta files. The optimization truncates large keys
after the first unique byte between sequential values. The deltafile
values, in the case of this integration test, include the small
DeltaKey, and the 8KiB updated value. As a result the BTree interior
nodes are being completely filled by only ~4 values (32KiB cblock size
by default). This makes the BTree far less effective, and means that the
full updated data is written many times. We expect fixing this will
improve performance for update-heavy workloads with large values (for
example, YCSB).

Unfortunately, fixing the issue is not quite as simple as enabling the
optimization for deltafiles, since in the normal course of seeking
through deltafiles during a scan, we deserialze the value index keys
into a DeltaKey. If the values are truncated this deserialization step
can fail. Instead, this patch adds overridable value index key encoding
to CFileWriter, and delta file overrides it to only encode the delta
key, which is a pair of variable-length integers.

Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
8 files changed, 72 insertions(+), 36 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8982 )

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 12 Jan 2018 21:33:30 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..

KUDU-2253 Deltafile on-disk size is 3x larger than expected

While looking into the performance of the integration test written for
KUDU-2251 (https://gerrit.cloudera.org/#/c/8951/ revision 6), Todd and I
found that the on-disk deltafiles written are about 3x larger than
expected. The culprit is an optimization in the CFile value index which
is turned off for delta files. The optimization truncates large keys
after the first unique byte between sequential values. The deltafile
values, in the case of this integration test, include the small
DeltaKey, and the 8KiB updated value. As a result the BTree interior
nodes are being completely filled by only ~4 values (32KiB cblock size
by default). This makes the BTree far less effective, and means that the
full updated data is written many times. We expect fixing this will
improve performance for update-heavy workloads with large values (for
example, YCSB).

Unfortunately, fixing the issue is not quite as simple as enabling the
optimization for deltafiles, since in the normal course of seeking
through deltafiles during a scan, we deserialze the value index keys
into a DeltaKey. If the values are truncated this deserialization step
can fail. Instead, this patch adds overridable value index key encoding
to CFileWriter, and delta file overrides it to only encode the delta
key, which is a pair of variable-length integers.

Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
8 files changed, 71 insertions(+), 36 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9015 )

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h
File src/kudu/util/os-util.h:

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/os-util.h@33
PS5, Line 33:   static_assert(std::is_signed::value, \
need to #include  in this header so that we get access to 
std::is_signed. Also whatever header has EINTR in it (not sure which one)


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc
File src/kudu/util/subprocess-test.cc:

http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@28
PS5, Line 28: #include 
per Alexey's comment please separate out the C includes from the C++ ones 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@312
PS5, Line 312: t_started = true;
still think we should add a short sleep here so that we ensure that the 
signal-sending starts before we call p.Start


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@313
PS5, Line 313: ASSERT_OK(p.Start());
nit: we have to use CHECK_OK here because gtest isn't thread-safe to run 
assertions from non-main threads


http://gerrit.cloudera.org:8080/#/c/9015/5/src/kudu/util/subprocess-test.cc@332
PS5, Line 332:   // Add delay to avoid user defined signal 2 error
but the "user defined signal 2 error" is the bug we're trying to provoke here. 
So if we are still seeing a crash due to it, then that means we haven't fully 
fixed the bug, right?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 5
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 20:56:24 +
Gerrit-HasComments: Yes

[kudu-CR] [docs] Added steps to update HMS after migrating to multiple Kudu masters

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8948 )

Change subject: [docs] Added steps to update HMS after migrating to multiple 
Kudu masters
..


Patch Set 14: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
Gerrit-Change-Number: 8948
Gerrit-PatchSet: 14
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 20:48:02 +
Gerrit-HasComments: No

[kudu-CR] [docs] Added steps to update HMS after migrating to multiple Kudu masters

[Alex Rodoni (Code Review)]

Hello Thomas Tauber-Marshall, Jean-Daniel Cryans, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [docs] Added steps to update HMS after migrating to multiple 
Kudu masters
..

[docs] Added steps to update HMS after migrating to multiple Kudu masters

Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
---
M docs/administration.adoc
1 file changed, 36 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
Gerrit-Change-Number: 8948
Gerrit-PatchSet: 14
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [docs] Added steps to update HMS after migrating to multiple Kudu masters

[Alex Rodoni (Code Review)]

Alex Rodoni has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8948 )

Change subject: [docs] Added steps to update HMS after migrating to multiple 
Kudu masters
..


Patch Set 13:

> (1 comment)

 > (1 comment)

Done - I think


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
Gerrit-Change-Number: 8948
Gerrit-PatchSet: 13
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 19:55:04 +
Gerrit-HasComments: No

[kudu-CR] [docs] Added steps to update HMS after migrating to multiple Kudu masters

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8948 )

Change subject: [docs] Added steps to update HMS after migrating to multiple 
Kudu masters
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8948/13/docs/patch
File docs/patch:

http://gerrit.cloudera.org:8080/#/c/8948/13/docs/patch@1
PS13, Line 1: diff --git a/docs/security.adoc b/docs/security.adoc
Remove this file from the commit.

Please make sure that reviews contain the things you intend for them to, eg. by 
running 'git status', 'git log', and 'git diff  ' prior to 
submitting, and if you're not sure then also by looking at the review in gerrit 
after submitting.

Since you don't need this file at all any more, you can remove it by running, 
from the root /kudu/ dir:
> git rm docs/patch
> git commit --amend
which will actually delete the file and then update the commit



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
Gerrit-Change-Number: 8948
Gerrit-PatchSet: 13
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 19:49:23 +
Gerrit-HasComments: Yes

[kudu-CR] Kudu-2208 Avoid system call interruption in Subprocess

[Jeffrey F. Lukman (Code Review)]

Jeffrey F. Lukman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9011 )

Change subject: Kudu-2208 Avoid system call interruption in Subprocess
..


Patch Set 1:

(12 comments)

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

http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@301
PS1, Line 301: thd
> nit: either abbreviate to just 't' or 'thr' would be more in line with what
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@302
PS1, Line 302: 0.5
> nit: why not just '1'?  Using non-integer numbers for /bin/sleep is conside
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@303
PS1, Line 303: subprocessThread
> nit: we use snake_case style in C++
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@305
PS1, Line 305: ASSERT_OK(p.Start());
> I think adding a short sleep here (eg 50ms) is a good idea to ensure that t
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@310
PS1, Line 310:   // create signal
> you aren't creating a signal here, but rather setting up a signal handler
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@317
PS1, Line 317:   // send at most 10 kill signals to subprocess
> why at most 10? Why not loop until we see that the subprocess has exited? I
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@319
PS1, Line 319:   int signalCounter = 0;
 :   bool hasError = false;
 :
> nit: snake_case
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@322
PS1, Line 322: if (signalCounter > 10 || hasError) break;
> why not just put this into the while condition above?
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@325
PS1, Line 325:   hasError = pthread_kill(thd, SIGUSR2) == 0;
> do we expect any errors from this? if not, could we just ASSERT this?
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess-test.cc@330
PS1, Line 330: SleepFor(MonoDelta::FromMilliseconds(10));
> why sleep in between? if our goal is to provoke races we want to send as ma
I added sleep time here to avoid undefined signal 2 exception.


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

http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess.cc@67
PS1, Line 67: // Retry on EINTR for functions like read() that return -1 on 
error.
> can we move this function to a utility header since it's used in a few plac
Done


http://gerrit.cloudera.org:8080/#/c/9011/1/src/kudu/util/subprocess.cc@69
PS1, Line 69:  == true
> nit: can we drop this extra?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I66ab2ec391eced5ea1c37d4294d151fe03c7d586
Gerrit-Change-Number: 9011
Gerrit-PatchSet: 1
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 12 Jan 2018 19:30:59 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..

KUDU-2208 Add unit test to detect Subprocess Interruption
Handling failure and patch to fix the bug

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Prior to the bug fix in this patch,
the Subprocess throws an exception because it cannot handle
the kill signal in Wait() state. The bug fix adds RETRY_ON_EINTR()
at Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 60 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 5
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] consensus: Fix KUDU-1735 regression test in 3-4-3 mode

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9013 )

Change subject: consensus: Fix KUDU-1735 regression test in 3-4-3 mode
..


Patch Set 1:

> Uploaded patch set 1.

I integrated and expanded the additional checks from this changelist into 
https://gerrit.cloudera.org/#/c/8989/ as a result of our offline discussion 
yesterday.  Also, more comments are added explaining the assertions in the test.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I275ba76c7fc7711e05b9dbda02a659d4245c40d0
Gerrit-Change-Number: 9013
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 12 Jan 2018 19:06:56 +
Gerrit-HasComments: No

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8989 )

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..


Patch Set 3:

> I posted a rough sketch of a fix and an explanation in
 > https://gerrit.cloudera.org/c/9013/

Great, thanks.  This patch already works with the specific of 3-4-3, I just 
wanted to make sure that's this is the legit behavior.

I integrated more perks from the 9013 item, and added explanation on why it 
works the way it's written.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 12 Jan 2018 19:02:40 +
Gerrit-HasComments: No

[kudu-CR] [raft consensus-itest] proper fix for Test KUDU 1735

[Alexey Serbin (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: [raft_consensus-itest] proper fix for Test_KUDU_1735
..

[raft_consensus-itest] proper fix for Test_KUDU_1735

Adapted the logic of the Test_KUDU_1735 scenario to work for both
3-2-3 and 3-4-3 replication schemes.

This is a follow-up for 8bcc80eec075321faef26b2a0ccac12ac9d7.

Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
---
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/util/fault_injection.cc
2 files changed, 89 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd7695f2b78da9755eb109940b40bda2bc6ec02e
Gerrit-Change-Number: 8989
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8982 )

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8982/2/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:

http://gerrit.cloudera.org:8080/#/c/8982/2/src/kudu/tablet/deltafile.cc@107
PS2, Line 107:   opts.optimize_index_keys = false;
> did you consider putting the function in WriterOptions instead of a new par
Done


http://gerrit.cloudera.org:8080/#/c/8982/2/src/kudu/tablet/deltafile.cc@109
PS2, Line 109:   cfile::ValidxKeyEncoder key_encoder = [] (const void* value, 
faststring* buffer) {
> worth a comment here explaining why we can truncate the index and why we ne
Done


http://gerrit.cloudera.org:8080/#/c/8982/2/src/kudu/tablet/deltafile.cc@117
PS2, Line 117:   writer_.reset(new cfile::CFileWriter(std::move(opts),
> warning: std::move of the variable 'opts' of the trivially-copyable type 'c
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 12 Jan 2018 18:46:30 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2253 Deltafile on-disk size is 3x larger than expected

[Dan Burkert (Code Review)]

Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2253 Deltafile on-disk size is 3x larger than expected
..

KUDU-2253 Deltafile on-disk size is 3x larger than expected

While looking into the performance of the integration test written for
KUDU-2251 (https://gerrit.cloudera.org/#/c/8951/ revision 6), Todd and I
found that the on-disk deltafiles written are about 3x larger than
expected. The culprit is an optimization in the CFile value index which
is turned off for delta files. The optimization truncates large keys
after the first unique byte between sequential values. The deltafile
values, in the case of this integration test, include the small
DeltaKey, and the 8KiB updated value. As a result the BTree interior
nodes are being completely filled by only ~4 values (32KiB cblock size
by default). This makes the BTree far less effective, and means that the
full updated data is written many times. We expect fixing this will
improve performance for update-heavy workloads with large values (for
example, YCSB).

Unfortunately, fixing the issue is not quite as simple as enabling the
optimization for deltafiles, since in the normal course of seeking
through deltafiles during a scan, we deserialze the value index keys
into a DeltaKey. If the values are truncated this deserialization step
can fail. Instead, this patch adds overridable value index key encoding
to CFileWriter, and delta file overrides it to only encode the delta
key, which is a pair of variable-length integers.

Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
8 files changed, 67 insertions(+), 36 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4cea3371fcf57f89fe10a3b9262bc152023cb04c
Gerrit-Change-Number: 8982
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and patch to fix the bug

[Jeffrey F. Lukman (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2208 Add unit test to detect Subprocess Interruption 
Handling failure and patch to fix the bug
..

KUDU-2208 Add unit test to detect Subprocess Interruption Handling failure and 
patch to fix the bug

This patch submits a unit test that create a Subprocess thread
and while it is starting and waiting, another thread sends kill signals
to the Subprocess thread. Prior to the bug fix in this patch,
the Subprocess throws an exception because it cannot handle
the kill signal in Wait() state. The bug fix adds RETRY_ON_EINTR()
at Subprocess::DoWait() function.

Furthermore, this patch also moves the RETRY_ON_EINTR() function
that occurs in many places across the code to os-util.h.

Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
---
M src/kudu/consensus/log_index.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/os-util.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
6 files changed, 57 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333
Gerrit-Change-Number: 9015
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon