[kudu-CR] [tools] select only tablet leader for RWYW

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [tools] select only tablet leader for RWYW
..


[tools] select only tablet leader for RWYW

To enable stable read-what-you-write behavior in case of multiple
replicas, use LEADER_ONLY selection for scan operations.

This is a follow-up for 5385af86dc61e56dbe601f0ad6c2bc8fba30ed7c.

Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
Reviewed-on: http://gerrit.cloudera.org:8080/4571
Reviewed-by: David Ribeiro Alves 
Tested-by: Alexey Serbin 
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 1 insertion(+), 0 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Alexey Serbin: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] [tools] select only tablet leader for RWYW

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tools] select only tablet leader for RWYW
..


Patch Set 1:

> Patch Set 1: Verified+1

Removed -1 by Jenkins because of unrelated failure of 
DeleteTableTest.TestAutoTombstoneAfterCrashDuringTabletCopy

Built at ve0518.halxg.cloudera.com and verified by running dist_test.py 1K 
iterations with kudu-tool-test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

[kudu-CR] [tools] select only tablet leader for RWYW

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tools] select only tablet leader for RWYW
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Also snuck in a small change to display host:port details with
'kudu table list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
5 files changed, 187 insertions(+), 44 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 8:

(1 comment)

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

PS2, Line 499: en to by another lbm, we must reinspect th
> Compaction writes new blocks to the container, and old ones are hole punche
Great, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


KUDU-1657: read-only FsManager::Open on active tablet can crash

This fixes a race condition when opening a read-only FsManager on a data
directory with lbm containers that are actively being appended to. See
the code comment for more details. A best-effort repro test case is
included; it typically takes about 35 seconds to repro without the fix.
This issue was identified because it was causing
alter_table-randomized-test to be significantly flaky.

Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Reviewed-on: http://gerrit.cloudera.org:8080/4551
Reviewed-by: Adar Dembo 
Tested-by: Dan Burkert 
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/open-readonly-fs-itest.cc
3 files changed, 158 insertions(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 7: Verified+1

Jenkins is completely hosed and I'm tired of waiting.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 4:

Please rebase this patch before pushing another revision.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tools] select only tablet leader for RWYW

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [tools] select only tablet leader for RWYW
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [tools] select only tablet leader for RWYW

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [tools] select only tablet leader for RWYW
..

[tools] select only tablet leader for RWYW

To enable stable read-what-you-write behavior in case of multiple
replicas, use LEADER_ONLY selection for scan operations.

This is a follow-up for 5385af86dc61e56dbe601f0ad6c2bc8fba30ed7c.

Change-Id: Ied2e7a6b7c31850e585c9d61655429d59c83ba89
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 1 insertion(+), 0 deletions(-)


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

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

[kudu-CR] KuduPredicate::Clone on IN list predicate causes segfault

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: KuduPredicate::Clone on IN list predicate causes segfault
..


KuduPredicate::Clone on IN list predicate causes segfault

Jordan Birdsell uncovered this issue where a cloned IN list predicate
causes a segfault when applied to a scanner. During the clone, the
values list was being 'preallocated' using the vector(int) constructor,
which adds a bunch of default constructed values (in this case, nullptrs
since the type is const void*). We previously didn't have any test
coverage of the Clone method, so I added a check to every column
predicate test to make sure that scans with cloned predicates behave the
same as the original predicates. The test segfaults without the fix.

Change-Id: I977691390535273c37aa90c3a3663a7c09ea67fc
Reviewed-on: http://gerrit.cloudera.org:8080/4568
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Jordan Birdsell 
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
2 files changed, 21 insertions(+), 3 deletions(-)

Approvals:
  Jordan Birdsell: 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/4568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I977691390535273c37aa90c3a3663a7c09ea67fc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 218:   tablet_id_
> But why not get the leader info from the consensus info too? Why go to two 
One reason I was hesitant to use the same routine for leadership info was 
because GetConsensus collects the details from all the tservers, and I wasn't 
sure whether one of them could end up returning a stale info depending on when 
the RPC hits them. In the code below at L234, probability of that RPC hitting 
when the leader failover is in transition is very likely. If anything, this was 
mainly due to not knowing the codebase well and staying on safer side(go to 
master for leader info).


PS1, Line 230: "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
> Now I see what you mean; the number of attempts in AssertEventually is neve
Well we don't want to include the  under AssertEventually, 
because it's perfectly possible that even after specified timeout, we never 
elected a leader which is different than current_leader. ASSERT_NE(new_leader, 
current_leader) could yield us incorrect results. I will still debug what I 
observed is just conincidence or there is more to it than just meet the eye. 
However, the ++attempts kinda ensures that eventually new_leader emerges on the 
other side.


http://gerrit.cloudera.org:8080/#/c/4533/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS3, Line 221: // Grab the default settings for heartbeat timeouts.
 : string hb_timeout_str;
 : 
ASSERT_TRUE(google::GetCommandLineOption("raft_heartbeat_interval_ms",
 :  _timeout_str));
 : int32 hb_timeout;
 : ASSERT_TRUE(safe_strto32(hb_timeout_str, _timeout));
 : string max_missed_hb_periods_str;
 : ASSERT_TRUE(
 : google::GetCommandLineOption(
 : "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
 : double max_missed_hb_periods;
 : ASSERT_TRUE(safe_strtod(max_missed_hb_periods_str,
 : _missed_hb_periods));
> You don't need to go through all this trouble (and you certainly don't need
Good point, forgot about DECLARE, done.


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

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

[kudu-CR] [java client] Tight-ish loop in master lookups if a tablet doesn't have a leader

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has uploaded a new change for review.

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

Change subject: [java client] Tight-ish loop in master lookups if a tablet 
doesn't have a leader
..

[java client] Tight-ish loop in master lookups if a tablet doesn't have a leader

There's currently the possiblity of a situation like this:
1. sendRpcToTablet finds the tablet that the RPC is going to doesn't have a 
leader
2. a master lookup is sent
3. discoverTablet is called back and still doesn't find a leader
4. RetryRpcCallback is invoked right away, going back to step 1

This quickly gets us with this exception:

Too many attempts: KuduRpc(method=Write, tablet=redacted, attempt=101, 
DeadlineTracker(timeout=3, elapsed=4747)

Notice how it retried 101 times in less than 5 seconds.

This patch changes step 3 so that an exception is thrown so that RetryRpcErrback
is invoked instead, which will add delay before retrying the RPC.

This bug was found by ITClient. It's not _just_ a flaky test!

Change-Id: Ibf2bd53b03551642e4d036d322e1e592b7c2cfec
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
A 
java/kudu-client/src/main/java/org/apache/kudu/client/NoSuitableReplicaException.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
3 files changed, 98 insertions(+), 19 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf2bd53b03551642e4d036d322e1e592b7c2cfec
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 

[kudu-CR] [c++ client] added few deprecation notes

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] added few deprecation notes
..

[c++ client] added few deprecation notes

KUDU-1661 Mark kudu::client::KuduClient::GetLatestObservedTimestamp()
  as experimental

Added deprecation notes for methods related to getting and setting
hybrid timestamps to perform READ_AT_SNAPSHOT scan:
  * KuduClient::GetLatestObservedTimestamp()
  * KuduClient::SetLatestObservedTimestamp()
  * KuduScanner::SetSnapshotRaw()

Change-Id: I6c45b797fa459ac9d214bb2612fd797f7a1eea45
---
M src/kudu/client/client.h
1 file changed, 18 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c45b797fa459ac9d214bb2612fd797f7a1eea45
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: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] added few deprecation notes

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] added few deprecation notes
..


Patch Set 1:

(1 comment)

> (1 comment)
 > 
 > Shouldn't we wait until the new API lands before deprecating the
 > old one though?

That's a good question.

The original intention for this change was to warn users of the API about the 
anticipated changes (as David mentioned in HipChat channel).

We could just add a notice, warning or attention (@notice, @warning and 
@attention in Doxygen lingo) as comments, but I thought if were  about to 
deprecate the methods pretty soon, I could just add the deprecation note along 
with corresponding clang/gcc attribute.

It would be nice to collect more feedback on this.

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

PS1, Line 396: its
> Nit: "in signature". Or maybe just "or change in a future release"
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c45b797fa459ac9d214bb2612fd797f7a1eea45
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] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [tools] added insert-generated-rows into kudu tools
..


[tools] added insert-generated-rows into kudu tools

The insert-generated-rows tool has been merged into the 'kudu'
umbrella toolset.

This addresses KUDU-1628.

Besides, added ability to run multiple inserter threads and
specify additional parameters on batching behavior of the generated
write operations.  It's possible to run data generating sessions
both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes.  Introduced
sequential and random modes for the data generator.  Overall, these
changes allow to use the tool to measure performance of the Kudu C++
client library in simplistic 'push-as-much-as-you-can' scenario:
the client generates and sends data as fast as it can.

Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Reviewed-on: http://gerrit.cloudera.org:8080/4412
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/insert-generated-rows.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
17 files changed, 751 insertions(+), 183 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 23
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 22: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 22
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] added few deprecation notes

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [c++ client] added few deprecation notes
..


Patch Set 1:

(1 comment)

Shouldn't we wait until the new API lands before deprecating the old one though?

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

PS1, Line 396: its
Nit: "in signature". Or maybe just "or change in a future release"

Do the same in the other methods, and in the @deprecated tags.


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

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

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [tools] added insert-generated-rows into kudu tools
..

[tools] added insert-generated-rows into kudu tools

The insert-generated-rows tool has been merged into the 'kudu'
umbrella toolset.

This addresses KUDU-1628.

Besides, added ability to run multiple inserter threads and
specify additional parameters on batching behavior of the generated
write operations.  It's possible to run data generating sessions
both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes.  Introduced
sequential and random modes for the data generator.  Overall, these
changes allow to use the tool to measure performance of the Kudu C++
client library in simplistic 'push-as-much-as-you-can' scenario:
the client generates and sends data as fast as it can.

Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/insert-generated-rows.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
17 files changed, 751 insertions(+), 183 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/4412/22
-- 
To view, visit http://gerrit.cloudera.org:8080/4412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 22
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 21:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4412/21/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:

PS21, Line 413:  = Status::OK();
> Nit: this is the default value, can omit.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 21
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] added few deprecation notes

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [c++ client] added few deprecation notes
..

[c++ client] added few deprecation notes

KUDU-1661 Mark kudu::client::KuduClient::GetLatestObservedTimestamp()
  as experimental

Added deprecation notes for methods related to getting and setting
hybrid timestamps to perform READ_AT_SNAPSHOT scan:
  * KuduClient::GetLatestObservedTimestamp()
  * KuduClient::SetLatestObservedTimestamp()
  * KuduScanner::SetSnapshotRaw()

Change-Id: I6c45b797fa459ac9d214bb2612fd797f7a1eea45
---
M src/kudu/client/client.h
1 file changed, 18 insertions(+), 3 deletions(-)


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

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

[kudu-CR] KuduPredicate::Clone on IN list predicate causes segfault

[Jordan Birdsell (Code Review)]

Jordan Birdsell has posted comments on this change.

Change subject: KuduPredicate::Clone on IN list predicate causes segfault
..


Patch Set 1: Code-Review+1

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

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

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 21:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4412/21/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:

PS21, Line 413:  = Status::OK();
Nit: this is the default value, can omit.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 21
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..

KUDU-1657: read-only FsManager::Open on active tablet can crash

This fixes a race condition when opening a read-only FsManager on a data
directory with lbm containers that are actively being appended to. See
the code comment for more details. A best-effort repro test case is
included; it typically takes about 35 seconds to repro without the fix.
This issue was identified because it was causing
alter_table-randomized-test to be significantly flaky.

Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/open-readonly-fs-itest.cc
3 files changed, 158 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KuduPredicate::Clone on IN list predicate causes segfault

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KuduPredicate::Clone on IN list predicate causes segfault
..


Patch Set 1: Code-Review+2

Would be great to just remove the Clone() API, or deprecate it or something.

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

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

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 20:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4412/20/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:

PS20, Line 401: supposed
> Nit: "assumed", right?
Done


PS20, Line 405:   // It's necessary to have read-what-you-write behavior here.  
Since
  :   // tablet leader can change and there might be replica 
propagation delays,
  :   // set the snapshot to the latest observed one to get correct 
row count.
> As JD discovered with ITClient, if you end up scanning from a replica that 
OK, thanks for the info.  I added retry logic for that.  It looks uglier now, 
but it should do the job.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 20
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [tools] added insert-generated-rows into kudu tools
..

[tools] added insert-generated-rows into kudu tools

The insert-generated-rows tool has been merged into the 'kudu'
umbrella toolset.

This addresses KUDU-1628.

Besides, added ability to run multiple inserter threads and
specify additional parameters on batching behavior of the generated
write operations.  It's possible to run data generating sessions
both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes.  Introduced
sequential and random modes for the data generator.  Overall, these
changes allow to use the tool to measure performance of the Kudu C++
client library in simplistic 'push-as-much-as-you-can' scenario:
the client generates and sends data as fast as it can.

Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/insert-generated-rows.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
17 files changed, 751 insertions(+), 183 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 21
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KuduPredicate::Clone on IN list predicate causes segfault

[Dan Burkert (Code Review)]

Hello Jordan Birdsell, Adar Dembo,

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

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

to review the following change.

Change subject: KuduPredicate::Clone on IN list predicate causes segfault
..

KuduPredicate::Clone on IN list predicate causes segfault

Jordan Birdsell uncovered this issue where a cloned IN list predicate
causes a segfault when applied to a scanner. During the clone, the
values list was being 'preallocated' using the vector(int) constructor,
which adds a bunch of default constructed values (in this case, nullptrs
since the type is const void*). We previously didn't have any test
coverage of the Clone method, so I added a check to every column
predicate test to make sure that scans with cloned predicates behave the
same as the original predicates. The test segfaults without the fix.

Change-Id: I977691390535273c37aa90c3a3663a7c09ea67fc
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
2 files changed, 21 insertions(+), 3 deletions(-)


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

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

[kudu-CR] Add a design doc for rpc retry/failover semantics

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a design doc for rpc retry/failover semantics
..

Add a design doc for rpc retry/failover semantics

This adds the final version of the rpc retry/failover doc and
includes details of the final implementation. It also includes
a guide and hints on how to implement exactly once semantics
for other RPCs.

Change-Id: Idc2aa40486153b39724e1c9bd09c626b829274c6
---
M docs/design-docs/README.md
A docs/design-docs/rpc-retry-and-failover.md
2 files changed, 235 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idc2aa40486153b39724e1c9bd09c626b829274c6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [util] Fix a minor bug in AssertEventually()

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [util] Fix a minor bug in AssertEventually()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 590: default: LOG(FATAL) << "unknown predicate type";
> Out of curiosity, does ToString() emit a warning too? If not, why not?
Hah, I had the same Qn when you suggested me to take a look at this routine, my 
wild guess was that LOG(FATAL) has some sorta exit code which probably ends up 
with an implicit typecast to string :). In theory, the warning should be caught 
on all non-void returns:
warning: control reaches end of non-void function [-Wreturn-type]


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

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

[kudu-CR] [WIP]KUDU-1640 - [python] Add IN-list predicate support

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [WIP]KUDU-1640 - [python] Add IN-list predicate support
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4548/3/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 812: del col_name_slice
> box_value can throw, right?  Should everything between new Slice and del co
actually, it seems like col_name_slice doesn't need to be heap allocated.  
Could you just move it into NewInListPredicate?

I am worried about what happens to the values already created if any of the 
calls to box_value throw; won't they be leaked?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 6:

(4 comments)

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

PS2, Line 499: en to by another lbm, we must reinspect th
> Sort of; the log block manager is careful to only write to the metadata fil
Hmmm, makes sense, thanks. And how does compaction weigh in here ? i.e if 
compaction kicked in after the check (data_file_sz < record.details) and end up 
reducing the data_file_size even further ? I am not sure if that's possible or 
not or perhaps compaction results into writing to a new data file altogether so 
that situation doesn't arise here ?


http://gerrit.cloudera.org:8080/#/c/4551/5/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

Line 30: using kudu::client::KuduColumnSchema;
Unused ?


Line 36: using kudu::client::KuduWriteOperation;
Unused ? Ignore these comments since I don't trust gerrit searches sometimes


Line 92:   }
80-char wrap up ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4551/5/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

PS5, Line 44: const auto kTimeout = MonoDelta::FromSeconds(60);
: const char kTableName[] = "test-table";
: const int kNumColumns = 50;
> huh?  They were at the top of the TEST_F in rev 3.
Yeah, but not with the kCamelCase variable name convention we use.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4551/5/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

PS5, Line 44: const char kTableName[] = "test-table";
: const int kNumColumns = 50;
: const auto kTimeout = MonoDelta::FromSeconds(60);
> Sorry, I meant at the top of the TEST_F() method itself, but I guess this i
huh?  They were at the top of the TEST_F in rev 3.


PS5, Line 59: flush blocks
> Nit: "flush blocks" and "compaction blocks" isn't wrong, but it suggests th
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [WIP]KUDU-1640 - [python] Add IN-list predicate support

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [WIP]KUDU-1640 - [python] Add IN-list predicate support
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4548/3/python/kudu/client.pyx
File python/kudu/client.pyx:

Line 812: del col_name_slice
box_value can throw, right?  Should everything between new Slice and del 
col_name_slice be in a try /finally?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I932dfded62e162cf85e0e12432cf6716311957de
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 5: Code-Review+2

+2 from me, once Adar's nits get addressed. Thanks for adding the test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] build-and-test.sh: update gcovr location

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: build-and-test.sh: update gcovr location
..


Patch Set 2: Verified+1

No coverage from gerrit here.

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

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

[kudu-CR] build-and-test.sh: update gcovr location

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: build-and-test.sh: update gcovr location
..


build-and-test.sh: update gcovr location

Coverage builds were failing because of this.

Change-Id: I8874d298b6176b4deb45749847032e5c2be55955
Reviewed-on: http://gerrit.cloudera.org:8080/4567
Reviewed-by: David Ribeiro Alves 
Tested-by: Adar Dembo 
---
M build-support/jenkins/build-and-test.sh
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Adar Dembo: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8874d298b6176b4deb45749847032e5c2be55955
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

[kudu-CR] build-and-test.sh: update gcovr location

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: build-and-test.sh: update gcovr location
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] [util] Fix a minor bug in AssertEventually()

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [util] Fix a minor bug in AssertEventually()
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 590: default: LOG(FATAL) << "unknown predicate type";
Out of curiosity, does ToString() emit a warning too? If not, why not?


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

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

[kudu-CR] build-and-test.sh: update gcovr location

[Adar Dembo (Code Review)]

Hello David Ribeiro Alves,

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

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

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

Change subject: build-and-test.sh: update gcovr location
..

build-and-test.sh: update gcovr location

Coverage builds were failing because of this.

Change-Id: I8874d298b6176b4deb45749847032e5c2be55955
---
M build-support/jenkins/build-and-test.sh
1 file changed, 1 insertion(+), 1 deletion(-)


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

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

[kudu-CR] [util] Fix a minor bug in AssertEventually()

[Dinesh Bhat (Code Review)]

Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [util] Fix a minor bug in AssertEventually()
..

[util] Fix a minor bug in AssertEventually()

We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.

Also snuck in a change to remove a warning stemming from fresh builds.

Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
---
M src/kudu/common/column_predicate.cc
M src/kudu/util/test_util.cc
2 files changed, 3 insertions(+), 2 deletions(-)


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

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

[kudu-CR] build-and-test.sh: update gcovr location

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: build-and-test.sh: update gcovr location
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] build-and-test.sh: update gcovr location

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: build-and-test.sh: update gcovr location
..


Patch Set 1: Verified+1

gerrit precommit tests don't cover this (perhaps ironically).

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

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

[kudu-CR] build-and-test.sh: update gcovr location

[Adar Dembo (Code Review)]

Adar Dembo has uploaded a new change for review.

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

Change subject: build-and-test.sh: update gcovr location
..

build-and-test.sh: update gcovr location

Coverage builds were failing because of this.

Change-Id: I8874d298b6176b4deb45749847032e5c2be55955
---
M build-support/jenkins/build-and-test.sh
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8874d298b6176b4deb45749847032e5c2be55955
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4551/5/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

PS5, Line 44: const char kTableName[] = "test-table";
: const int kNumColumns = 50;
: const auto kTimeout = MonoDelta::FromSeconds(60);
Sorry, I meant at the top of the TEST_F() method itself, but I guess this is OK 
too.


PS5, Line 59: flush blocks
Nit: "flush blocks" and "compaction blocks" isn't wrong, but it suggests that 
there is a difference in blocks between flushes and compactions, which isn't 
really the case. How about "we reduce the MRS flush threshold to increase flush 
frequency and increase the number of MM threads to encourage frequent 
compactions. The net effect of both of these changes: more blocks are written 
to disk."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [docs] Cleanup beta mentions, links

[Jean-Daniel Cryans (Code Review)]

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

Change subject: [docs] Cleanup beta mentions, links
..


[docs] Cleanup beta mentions, links

Change-Id: I56fa65be9d027f5b40628aa5db5f9e4a57c1fbc9
Reviewed-on: http://gerrit.cloudera.org:8080/4565
Reviewed-by: David Ribeiro Alves 
Tested-by: Kudu Jenkins
---
M docs/administration.adoc
M docs/configuration.adoc
M docs/index.adoc
3 files changed, 5 insertions(+), 7 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I56fa65be9d027f5b40628aa5db5f9e4a57c1fbc9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [docs] Release note for OperationResponse.getWriteTimestamp
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4551/3/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

PS3, Line 54: // To repro KUDU-1657, many flushes need to be happening, so 
turn knobs in
: // order to write and flush as fast as possible.
> Nit: that explains flags #2 and #3, but not #1. For #1, I think it's becaus
Done


PS3, Line 80:   int num_columns = 50;
:   auto timeout = MonoDelta::FromSeconds(60);
> Nit: maybe declare these at the top of the test with kFoo syntax, so it's c
Done


Line 91:   CHECK_OK(b.Build());
> Nit: could be ASSERT_OK since it's in a test.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..

KUDU-1657: read-only FsManager::Open on active tablet can crash

This fixes a race condition when opening a read-only FsManager on a data
directory with lbm containers that are actively being appended to. See
the code comment for more details. A best-effort repro test case is
included; it typically takes about 35 seconds to repro without the fix.
This issue was identified because it was causing
alter_table-randomized-test to be significantly flaky.

Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/open-readonly-fs-itest.cc
3 files changed, 157 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4551/3/src/kudu/integration-tests/open-readonly-fs-itest.cc
File src/kudu/integration-tests/open-readonly-fs-itest.cc:

PS3, Line 54: // To repro KUDU-1657, many flushes need to be happening, so 
turn knobs in
: // order to write and flush as fast as possible.
Nit: that explains flags #2 and #3, but not #1. For #1, I think it's because 
you don't want the test to eat gobs of disk space right? Or was it because you 
wanted container file sizes to change as often as possible?


PS3, Line 80:   int num_columns = 50;
:   auto timeout = MonoDelta::FromSeconds(60);
Nit: maybe declare these at the top of the test with kFoo syntax, so it's clear 
they're constants that control the performance characteristics of the test?


Line 91:   CHECK_OK(b.Build());
Nit: could be ASSERT_OK since it's in a test.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [util] Fix a minor bug in AssertEventually()

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [util] Fix a minor bug in AssertEventually()
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4566/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 591:   return false;
Hmm. If we add a new PredicateType but forget to update this function, couldn't 
that lead to really strange errors at runtime? Can we stifle the warning in 
some other way, perhaps by adding a catch-all LOG(FATAL) as was done in 
ToString()?


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

Line 189:   while (MonoTime::Now() < deadline) {
Might be simpler as a for loop that initializes attempts to 0, checks the 
deadline, and increments attempts.


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

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

[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [docs] Release note for OperationResponse.getWriteTimestamp
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..


Patch Set 2:

(3 comments)

Test added in slow mode.

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

Line 496: if (data_file_size < record.offset() + record.length()) {
> PREDICT_FALSE to annotate that this is a rare race
Done


PS2, Line 499: in order to account for the latest appends
> Good find Dan, this is not a review but a curiosity Qn: Looking at the desc
Sort of; the log block manager is careful to only write to the metadata file 
after writing the data file.  So if the other thread or process that's reading 
the metadata file in read-only mode sees an entry for a block in the metadata 
file, it should be guaranteed that if it then rereads the data file length, the 
length will be past the end of the block.


PS2, Line 504: local_records.back()
> Nit: replace with record ?
Done


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

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

[kudu-CR] KUDU-1657: read-only FsManager::Open on active tablet can crash

[Dan Burkert (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1657: read-only FsManager::Open on active tablet can crash
..

KUDU-1657: read-only FsManager::Open on active tablet can crash

This fixes a race condition when opening a read-only FsManager on a data
directory with tablets that are actively flushing. See the code comment
for more details.  A best-effort repro test case is included; it
typically takes about 35 seconds to repro without the fix. This issue
was identified because it was causing alter_table-randomized-test to be
significantly flaky.

Change-Id: If68b04f1f1b8cd099120a220b1245ecf8f422770
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/open-readonly-fs-itest.cc
3 files changed, 154 insertions(+), 1 deletion(-)


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

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

[kudu-CR] [docs] Release note for OperationResponse.getWriteTimestamp

[Jean-Daniel Cryans (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [docs] Release note for OperationResponse.getWriteTimestamp
..

[docs] Release note for OperationResponse.getWriteTimestamp

See https://gerrit.cloudera.org/#/c/4487/

Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0
---
M docs/release_notes.adoc
1 file changed, 4 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f81b1c70bc8f8be7f5529762ac9a9622dec00f0
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 20:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4412/20/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:

PS20, Line 401: supposed
Nit: "assumed", right?


PS20, Line 405:   // It's necessary to have read-what-you-write behavior here.  
Since
  :   // tablet leader can change and there might be replica 
propagation delays,
  :   // set the snapshot to the latest observed one to get correct 
row count.
As JD discovered with ITClient, if you end up scanning from a replica that is 
behind the desired timestamp, it'll only wait a certain amount of time to catch 
up before timing out the scan. You may have to work around that here.

See KUDU-1656 for more details.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 20
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [tools] added insert-generated-rows into kudu tools
..

[tools] added insert-generated-rows into kudu tools

The insert-generated-rows tool has been merged into the 'kudu'
umbrella toolset.

This addresses KUDU-1628.

Besides, added ability to run multiple inserter threads and
specify additional parameters on batching behavior of the generated
write operations.  It's possible to run data generating sessions
both in MANUAL_FLUSH and AUTO_FLUSH_BACKGROUND modes.  Introduced
sequential and random modes for the data generator.  Overall, these
changes allow to use the tool to measure performance of the Kudu C++
client library in simplistic 'push-as-much-as-you-can' scenario:
the client generates and sends data as fast as it can.

Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/insert-generated-rows.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_action_remote_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
17 files changed, 722 insertions(+), 183 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 19
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] thirdparty: stifle unused argument warnings when building with clang

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: thirdparty: stifle unused argument warnings when building with 
clang
..


thirdparty: stifle unused argument warnings when building with clang

Dan tested my various thirdparty patches on macOS and reported that clang
emits thousands of unused argument warnings when building llvm, gflags, and
gtest. The first is due to the use of EXTRA_LDFLAGS in the llvm build; the
others have always been there.

We can tackle this in one of two ways:
1. Add -Qunused-arguments to EXTRA_CXXFLAGS. This isn't as easy as it
   sounds, because we need to restrict this to clang-based builds (gcc
   doesn't recognize the parameter), and sometimes that happens via CC/CXX
   environment variables and other times implicitly.
2. Narrow our prolific additions of -L... and -Wl,-rpath,... such that
   they're only added when needed.

I chose approach #2, which also meant remembering all of our
interdependencies. As best I can tell, here is the complete list:
- glog depends on gflags
- glog depends on libunwind
- pmemobj depends on pmem
- bitshuffle depends on lz4

With such a short list, approach #2 isn't actually that bad. I tested it by
building thirdparty with my system's clang. It worked once I disabled
-Werror in the nvml build.

I tried very hard not to regress commit 2567ed0. My system should be using
gold, though I noticed that only the shared objects in
installed/tsan had RUNPATH entries; the ones in installed/uninstrumented and
installed/common used RPATH. Nevertheless, I ran both debug and tsan tests,
which should have exercised all of them.

Change-Id: I41db7fa29c6823c2914881e3fe91844b0c315779
Reviewed-on: http://gerrit.cloudera.org:8080/4514
Reviewed-by: Dan Burkert 
Tested-by: Dan Burkert 
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
2 files changed, 36 insertions(+), 22 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved; Verified



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

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

[kudu-CR] c++ client: stop requiring the old gcc ABI

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: c++ client: stop requiring the old gcc ABI
..


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

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

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

[kudu-CR] thirdparty: build C dependencies with TSAN instrumentation too

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: build C dependencies with TSAN instrumentation too
..


Patch Set 1: Verified+1

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

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

[kudu-CR] thirdparty: build C dependencies with TSAN instrumentation too

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: thirdparty: build C dependencies with TSAN instrumentation too
..


thirdparty: build C dependencies with TSAN instrumentation too

Our C library dependencies are pretty small so this isn't a huge hit, and
it makes the sanitizer-based dependency stack much more consistent. Now,
'common' is only for tools and header-only dependencies, while
uninstrumented/tsan/... include every library.

With additional sanitization come new suppressions. I didn't spend a whole
lot of time trying to figure out which were legitimate.

Change-Id: Ie8274ff7bc57016733cdd3c26858483f74e68faf
Reviewed-on: http://gerrit.cloudera.org:8080/4559
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
Tested-by: Dan Burkert 
---
M build-support/tsan-suppressions.txt
M cmake_modules/FindPmem.cmake
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
4 files changed, 144 insertions(+), 121 deletions(-)

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



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

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

[kudu-CR] thirdparty: stifle unused argument warnings when building with clang

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: stifle unused argument warnings when building with 
clang
..


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

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

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

[kudu-CR] thirdparty: split into dependency groups

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: thirdparty: split into dependency groups
..


thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- tsan: dependencies that may be instrumented, and are indeed in this build
  (with -fsanitize=thread).

These three generally map to the existing "common", "uninstrumented", and
"tsan" thirdparty subdirectories. There's an obvious pattern here for future
sanitizer builds (e.g. MSAN would provide an "msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Reviewed-on: http://gerrit.cloudera.org:8080/4513
Reviewed-by: Dan Burkert 
Tested-by: Dan Burkert 
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 271 insertions(+), 202 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] thirdparty: split into dependency groups

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: split into dependency groups
..


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

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

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

[kudu-CR] thirdparty: fix up libtool scripts if needed

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: thirdparty: fix up libtool scripts if needed
..


thirdparty: fix up libtool scripts if needed

Older versions of libtool (such as the version found on el6) do not pass
-stdlib=libc++ found in CXXFLAGS down to the libtool link command line, and
as a result, the shared object is linked against the system libstdc++. The
net effect is that some thirdparty shared objects sprout @@GLIBCXX
dependencies. Mysteriously, Kudu libraries and binaries themselves do not,
and the @@GLIBCXX dependencies appear to be satisfied via libc++ somehow.

Nevertheless, this is confusing and could prove problematic down the road,
so here's a quick hack to "fix up" libtool scripts after they are generated
via configure.

Change-Id: Id51c10d38984e892496621135634d21f3e2386ce
Reviewed-on: http://gerrit.cloudera.org:8080/4512
Reviewed-by: Dan Burkert 
Tested-by: Dan Burkert 
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
A thirdparty/postflight.py
3 files changed, 136 insertions(+), 0 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id51c10d38984e892496621135634d21f3e2386ce
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] thirdparty: fix up libtool scripts if needed

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: fix up libtool scripts if needed
..


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

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

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

[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds
..


thirdparty: use libc++ instead libstdc++ for TSAN builds

This all began because I wanted two things:
1. To use the new gcc 5 ABI on platforms that default to it (such as Ubuntu
   Xenial). Other applications compiled on these platforms will use the new
   ABI, and the fact that the Kudu client forces them to use the old ABI is
   quite unfriendly.
2. To have working local TSAN builds again, which broke following the gcc 5
   ABI transition in Xenial.

There are a number of interconnected issues at play:
A. Until 3.9, LLVM did not recognize gcc's new ABI tags, which prevented
   Kudu's codegen module from building properly against the new ABI.
B. For TSAN builds, we rebuild some thirdparty dependencies against the
   libstdc++ from thirdparty, but the LLVM libraries are not one of them.
   This may work when the system libstdc++ and the thirdparty libstdc++ are
   of the same version, but becomes increasingly untenable as the versions
   differ. Why? Because libstdc++ only guarantees forward compatibility;
   that is, a binary linked against libstdc++ can only be used with a
   libstdc++ of the same version or newer. Attempts to run with an older
   libstdc++ can lead to run time errors about missing GLIBCXX symbols. As
   a point of reference, on Xenial the two libraries are more than a major
   version apart.
C. Continuing B, libstdc++ from gcc 5 actually breaks backward compatibility
   for certain C++11-only symbols by moving them to an inline namespace
   (e.g. std::error_category is now std::_V2::error_category). The LLVM
   libraries use these symbols, which means LLVM built against a gcc 5
   libstdc++ cannot link against the older libstdc++ in thirdparty.
D. As the libstdc++ in thirdparty is from gcc 4, it is not multilib and does
   not provide new ABI symbols (e.g. std::__cxx11::string). Meaning, if the
   rest of Kudu tried to use the new ABI, TSAN builds would fail because the
   libstdc++ in use lacks new ABI symbols.

After upgrading LLVM, the path of least resistance was to upgrade libstdc++
in thirdparty, but what a saga that turned out to be. After much trial and
error, I gave up; I could not build libstdc++ from gcc 5 with clang, and we
must use clang to realize the latest -fsanitize=thread support.

Are there any alternatives? Well, we can follow Chromium's lead and use
libc++ for TSAN instead of libstdc++. I think this makes sense for several
reasons:
- The LLVM build, such as it is, is much more friendly than gcc's build.
  Building libstdc++ out of all of gcc was always a little hacky.
- There's at least one large open-source project (Chromium) that's
  successfully gone down this path.

That brings us to this patch, which is largely about replacing libstdc++
with libc++. Here are additional interesting details:
o We now build entire set of TSAN-duplicated dependencies with
  -fsanitize=thread, not just protobuf. It doesn't affect correctness much
  either way, but it's simpler and an easier concept to extend to future
  sanitizers that DO care (e.g. MSAN).
o We now build LLVM twice: once against the system libstdc++ for build tools
  and the regular LLVM libraries, and a second time against libc++ for
  instrumented LLVM libraries. The first build is a little hokey: it'd be
  more "pure" to build LLVM three times: once for build tools, once for LLVM
  libraries, and once for instrumented LLVM libraries. But these builds are
  super long so we optimize by combining the first two. The downside is that
  the first build now places build tools in 'installed-deps' instead of
  'installed'. I played around with placing build tools in 'installed' while
  placing the libraries in 'installed-deps', but found that to be too hacky.
o The full thirdparty build is now quite a bit longer on account of the
  second LLVM library build. I tried to mitigate this by reducing the number
  of extra cruft built each time. An upcoming patch will address this
  further by splitting thirdparty into separate modules.
o libc++ depends on libc++abi, so we build that first.
o The libc++ and libc++abi builds are done standalone rather than with the
  LLVM library build, because it isn't possible to do them together AND have
  the LLVM libraries depend on libc++.
o build_python may now be invoked more than once, so I've changed it to be
  idempotent within the same run of build-thirdparty.sh.

Change-Id: Id9e68126ae21e04469053009c5b3e4b588415895
Reviewed-on: http://gerrit.cloudera.org:8080/4511
Reviewed-by: Dan Burkert 
Tested-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run-test.sh
M build-support/run_dist_test.py
M cmake_modules/FindPmem.cmake
M src/kudu/codegen/CMakeLists.txt
M thirdparty/build-definitions.sh
M 

[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] thirdparty: use libc++ instead libstdc++ for TSAN builds

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: use libc++ instead libstdc++ for TSAN builds
..


Patch Set 3: Verified+1

do it

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

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

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

Line 218:   tablet_id_
> sure, removed the comparison check, relying on master to report leader info
But why not get the leader info from the consensus info too? Why go to two 
different places to get the combined set of information? That just opens the 
door for discrepancies between the two.


PS1, Line 230: "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
> Sorry for not being clear before. Lemme try to explain although I haven't r
Now I see what you mean; the number of attempts in AssertEventually is never 
actually incremented. That's a pretty serious problem; can you fix that, and 
rebase this patch on top of the fix? No unit test needed.

As for the correct use of AssertEventually, let's get to the bottom of it as it 
should work. I can imagine something like this working:

  string current_leader = ...
  int64_t current_term = ...
  AssertEventually([&]() {

GetInfoFromConsensus(_leader, _term)
ASSERT_NE(new_leader, current_leader)
ASSERT_GT(new_term, current_term)
  });

It's possible that the broken backoff behavior in AssertEventually is 
responsible, which is why I suggested you fix the bug and rebase this patch on 
top of it. If not, use gdb or temporary logging to figure out what's going on.


http://gerrit.cloudera.org:8080/#/c/4533/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS3, Line 221: // Grab the default settings for heartbeat timeouts.
 : string hb_timeout_str;
 : 
ASSERT_TRUE(google::GetCommandLineOption("raft_heartbeat_interval_ms",
 :  _timeout_str));
 : int32 hb_timeout;
 : ASSERT_TRUE(safe_strto32(hb_timeout_str, _timeout));
 : string max_missed_hb_periods_str;
 : ASSERT_TRUE(
 : google::GetCommandLineOption(
 : "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
 : double max_missed_hb_periods;
 : ASSERT_TRUE(safe_strtod(max_missed_hb_periods_str,
 : _missed_hb_periods));
You don't need to go through all this trouble (and you certainly don't need to 
do it in a loop; it won't change during the loop). Just access 
FLAGS_raft_heartbeat_interval_ms and 
FLAGS_leader_failure_max_missed_heartbeat_periods directly, and  DECLARE_* them 
at the top of the file.


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

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

[kudu-CR] thirdparty: reorganize tree

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: thirdparty: reorganize tree
..


thirdparty: reorganize tree

This patch changes the organization of the thirdparty tree. The new layout
looks like this:
- installed: _all_ installed dependencies, with 'common',
 'uninstrumented', and 'tsan' subdirectories.
- build: build directories for all dependencies.
- src: source directories for all dependencies.

Additionally, the patch changes the build logic for each dependency so that
its build output is fully isolated from its source directory, and from other
build output (if the dependency is built multiple times).

Why do this?
1. Build isolation simplifies building dependencies multiple times (i.e. for
   different sanitizers) and makes it much safer.
2. It also means cleaning up build output doesn't mean redownloading all of
   the sources (i.e. no need to 'git clean -xdf thirdparty').
3. The grouping of all installed locations under the shared 'installed'
   subdirectory makes it a tad easier to blow it all away.
4. It also eases the transition for the remaining LLVM and thirdparty
   patches, as the conflicting build output will be copied to a different
   set of directories.
5. It slims down thirdparty/.gitignore; adding a new dependency no longer
   requires updating .gitignore.

Change-Id: I06aa75ab5e81f2563986244950e9dcda06c2d383
Reviewed-on: http://gerrit.cloudera.org:8080/4550
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M .ycm_extra_conf.py
M CMakeLists.txt
M README.adoc
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/lint.sh
M build-support/run_dist_test.py
M docs/installation.adoc
M docs/support/scripts/make_site.sh
M java/kudu-client/pom.xml
M src/kudu/client/client_samples-test.sh
M src/kudu/scripts/benchmarks.sh
M src/kudu/scripts/tpch.sh
M thirdparty/.gitignore
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 334 insertions(+), 217 deletions(-)

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



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

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

[kudu-CR] [tools] added insert-generated-rows into kudu tools

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools
..


Patch Set 18: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 18
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: [tools] Implement a manual leader_step_down for a tablet
..


Patch Set 3:

(9 comments)

TFTR Adar, pls see responses inline, also pls see why my jenkins are still 
failing. I see that they are still using ...llvm-3.8.0.src/ as per the logs.

http://gerrit.cloudera.org:8080/#/c/4533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS1, Line 204: string c
> The CHECK_* macros come from glog, and they all do the same thing regardles
Thank you for that awesome clarity on when to use them. This has gone in my 
favorite notes :)


Line 218:   tablet_id_
> Unfortunately a test environment with leader failure detection isn't determ
sure, removed the comparison check, relying on master to report leader info 
now. I have kept the getconsensus intact to grab the new term info.


PS1, Line 230: "leader_failure_max_missed_heartbeat_periods",
 : _missed_hb_periods_str));
> Couple things:
Sorry for not being clear before. Lemme try to explain although I haven't root 
caused the issue myself yet. When I used AssertEventually the GetConsensus 
comes out around 1.5 secs or so with a new_term. But what is not clear to me 
yet is, if I sleep for ~1.5 secs to allow the consensus to settle down, I see 
eventually a new_leader emerging which won't happen if I keep using 
GetConsensus via AssertEventually(despite 20 retries). Logs are confimring that 
GetConsensus indeed is detecting the new leader correctly, it's just that 
new_leader happens to be the current_leader everytime. 

I want to see why I see same leader gets elected again and again unless I allow 
some more time window before calling GetConsensus in which case I see 
new_leader coming up after few retries. With the fix of '++attempts' however, I 
eventually see another leader coming up so thats kinda hiding the problem. I 
think that ++attempts fix should have been there to keep the sleep time 
monotonically increasing.


PS1, Line 241: ASSERT_OK(GetTermFromConsensus(tservers, tablet_id_,
 :_ter
> Again, the leader can change whenever, so this doesn't seem safe.
Done


Line 264: 
> The actual check (not in the CHECK sense of the word, but the comparison an
yeah, missed out removing here i think. done.


http://gerrit.cloudera.org:8080/#/c/4533/2/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS2, Line 251: AGS_num_
> Still got some leftover CHECK_* statements that should be converted to thei
Done, thanks for explaining the distinctions earlier.


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

PS1, Line 274: 
 : 
> You missed these comments down here.
Ugh... sorry, for some reason I totally missed out scrolling down thinking that 
only change I had introduced was in the routine above :)


PS1, Line 278: 
 : 
> I admit, this one was my bad, but can you consolidate all of these descript
Done


PS1, Line 280: 
> Likewise for this, it's copied extensively.
Done


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

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

[kudu-CR] [tools] Implement a manual leader step down for a tablet

[Dinesh Bhat (Code Review)]

Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [tools] Implement a manual leader_step_down for a tablet
..

[tools] Implement a manual leader_step_down for a tablet

This change introduces a leader_step_down functionality
under 'kudu tablet'. This tool may be handy to recover from
situations when a single tablet server is overloaded and we want
to kick off a new election to balance the load across the clusters.
Although it is not guaranteed that a different replica will be elected
as the leader, this is an optimistic effort to elect a new tablet
server as the leader for the given tablet in the cluster.

Also snuck in a small change to display host:port details with
'kudu table list  --list_tablets' command.

Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/util/test_util.cc
6 files changed, 198 insertions(+), 42 deletions(-)


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

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