[kudu-CR] update dist-test configuration for new client.py location

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: update dist-test configuration for new client.py location
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/1773/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I48e4617222b8b503b21eeb560800dc5619d7ac26
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] update dist-test configuration for new client.py location

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: update dist-test configuration for new client.py location
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/1770/

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

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

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..

[java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

The testDisconnect test creates a single tablet table, scans the table,
disconnects a random tablet client, and then waits until the client has no
tablet clients. Since the Java client considers the master a tablet, this lasts
until the remaining tablet client times out. This commit changes the test to
instead disconnect all tablet clients, which shaves about 10 seconds off the
runtime.  There are also a few drive-by fixups, including renaming
AsyncKuduClient.getTableClients to getTabletClients.

The test fix exposed an existing race condition in the TabletClient
disconnect/reconnect handling which would cause an RPC to be 'lost' and never
be sent (eventually causing a timeout).

Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/ITClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
6 files changed, 17 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] update dist-test configuration for new client.py location

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Adar Dembo,

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

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

to review the following change.

Change subject: update dist-test configuration for new client.py location
..

update dist-test configuration for new client.py location

Change-Id: I48e4617222b8b503b21eeb560800dc5619d7ac26
---
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
2 files changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I48e4617222b8b503b21eeb560800dc5619d7ac26
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/1772/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3329/1/java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java:

Line 93: for (TabletClient tabletClient : client.getTabletClients()) {
> Instead, check that the port you get isn't the master's. BaseKuduTest offer
Not shutting down the master makes the while loop logic quite a bit more 
complicated.  I've changed the comments.  Not sure it really matters anyway, 
since it *is* disconnecting the TS.


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

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

[kudu-CR] WIP: KUDU-1466: improve error message when writes fail at TS

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: KUDU-1466: improve error message when writes fail at TS
..


Patch Set 1:

(1 comment)

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

PS1, Line 22: But,
: perhaps it's actually better for this to be done in the retriable 
RPC.
I'm supportive of the idea, but I'm not sure how it's best implemented.

Do we use a policy that prefers one kind of error over another (i.e. 
Status::TimedOut is always less interesting than other failures)? If we do 
something context-free like that, it should be sufficient to pass the "last 
error" around over the course of the operation, updating it whenever we see an 
error of higher priority.

Or, do we prefer one error over another based on the operational phase that it 
occurred (i.e. in tserver operations, lookup failures are always less 
interesting than actual tserver failures)? This approach suggests we find the 
appropriate "top-level" object for each operation (i.e. for scans, the 
KuduScanner itself), track the best error there, and make sure it's available 
to all phases of the operation to update if necessary.

For more context, we've already got "last" error tracking in KuduScanner::Data 
and RpcRetrier. If we're going to add it to a third location, let's choose 
deliberately and understand how all three work together.

The location picker isn't the worst place; it'll make the error available for 
scans and writes, the main culprits. But it'd be nice to make it available in 
administrative operations too, which are a little more ad-hoc at the moment.


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

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

[kudu-CR] update dist-test configuration for new client.py location

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: update dist-test configuration for new client.py location
..

update dist-test configuration for new client.py location

client.py has been moved to bin/client

Change-Id: I48e4617222b8b503b21eeb560800dc5619d7ac26
---
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
2 files changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48e4617222b8b503b21eeb560800dc5619d7ac26
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3329/2/java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java:

Line 57: // 2. Disconnect the client.
We're disconnecting all of them, not just the one client we want to disconnect. 
That's why I was in favor of changing the method's name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1353: remove per-tablet replica locations cache

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1353: remove per-tablet replica locations cache
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/2887/9/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 27: #include 
is this supposed to be in brackets?


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

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

[kudu-CR] KUDU-1353: remove per-tablet replica locations cache

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1353: remove per-tablet replica locations cache
..


Patch Set 10: Verified+1

Overriding Jenkins, the one failure was a Java test (testDisconnect) in TSAN 
mode. Dan has been working on that one.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d
Gerrit-PatchSet: 10
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] KUDU-1473: fix some tablet lock usage in CatalogManager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..


Patch Set 5: Verified+1

Overriding Jenkins, the one failure was a Java test (testDisconnect) in TSAN 
mode. Dan has been working on that one.

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

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

[kudu-CR] KUDU-1473: fix some tablet lock usage in CatalogManager

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..

KUDU-1473: fix some tablet lock usage in CatalogManager

This was probably due to the refactoring done in commit 59ff89d. Now that
we're releasing tablet write locks as early as possible, we need to
reacquire them (in read mode) for some operations.

The lock misuse reared its head most often in the Java test
TestKuduTable.testGetLocations, but could trigger in just about any test
that didn't wait for table creation to finish before accessing the table.
For example, CreateTableITest.TestCreateWhenMajorityOfReplicasFailCreation
was a little flaky too. The backtrace was always the same:

cow_object.h:82] Check failed: lock_.HasReaders() || lock_.HasWriteLock() 
@ 0x7fecb643e37b  kudu::CowObject<>::state() at ??:0
@ 0x7fecb6422fe9  
kudu::master::CatalogManager::ProcessPendingAssignments() at ??:0
@ 0x7fecb6421d3a  kudu::master::CatalogManagerBgTasks::Run() at ??:0
@ 0x7fecb645c1f7  boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7fecb645c15b  boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7fecb645c104  boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7fecb645bf2a  
boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7fecb0b66d82  boost::function0<>::operator()() at ??:0
@ 0x7fecafba96c0  kudu::Thread::SuperviseThread() at ??:0
@   0x423faa  __tsan_thread_start_func at ??:0
@ 0x7fecb28459d1  start_thread at ??:0
@ 0x7fecacd108fd  clone at ??:0

Change-Id: I8e24f6035f4d778995ea3f295396f5fbd760d6c6
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
2 files changed, 41 insertions(+), 26 deletions(-)


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

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

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/1776/

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

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

[kudu-CR] [java-client] RPC timeout may sometimes be reported as max attempts violation

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: [java-client] RPC timeout may sometimes be reported as max 
attempts violation
..


[java-client] RPC timeout may sometimes be reported as max attempts violation

Fixes an issue where an RPC timeout could be reported in the error message as
too many attempts.

Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
Reviewed-on: http://gerrit.cloudera.org:8080/3330
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
1 file changed, 5 insertions(+), 4 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1473: fix some tablet lock usage in CatalogManager

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..

KUDU-1473: fix some tablet lock usage in CatalogManager

This was probably due to the refactoring done in commit 59ff89d. Now that
we're releasing tablet write locks as early as possible, we need to
reacquire them (in read mode) for some operations.

It's still a mystery why the Java tests sometimes trigger the locking
violations and the C++ tests don't. Per the backtrace in the bug report, any
GetTableLocations() call to a table whose tablets have been assigned but are
otherwise still under construction may trigger a violation, as
GetTableLocations() acquires a tablet lock in read mode.

Change-Id: I8e24f6035f4d778995ea3f295396f5fbd760d6c6
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
2 files changed, 41 insertions(+), 26 deletions(-)


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

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

[kudu-CR] ts itest-base.h: wait for bootstrapping to finish when waiting for replicas

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: ts_itest-base.h: wait for bootstrapping to finish when waiting 
for replicas
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/1780/

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

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

[kudu-CR] KUDU-1353: remove per-tablet replica locations cache

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1353: remove per-tablet replica locations cache
..


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/1778/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d
Gerrit-PatchSet: 10
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] KUDU-1353: remove per-tablet replica locations cache

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1353: remove per-tablet replica locations cache
..

KUDU-1353: remove per-tablet replica locations cache

This is the simpler alternative to rebuilding the caches on metadata load.
Instead of using the caches to retrieve tserver RPC addresses, we'll look
the addresses up on the spot using the TSDescriptor cache. The two caches
should be equally coherent, as both were populated as a result of the master
receiving TS heartbeats.

I've also done away with the concept of a 'stale' locations response. The
distinction can be real, but it's confusing, and more importantly, clients
never took advantage of it. And if we're going to persist TSDescriptor data
in master state as has been recently discussed, the distinction becomes even
less useful.

Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d
---
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/master/master-path-handlers.h
M src/kudu/master/master-test-util.h
M src/kudu/master/master.proto
M src/kudu/master/ts_manager.cc
M src/kudu/master/ts_manager.h
M src/kudu/tools/create-demo-table.cc
13 files changed, 174 insertions(+), 306 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/2887/10
-- 
To view, visit http://gerrit.cloudera.org:8080/2887
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d
Gerrit-PatchSet: 10
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] KUDU-1353: remove per-tablet replica locations cache

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1353: remove per-tablet replica locations cache
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/2887/9/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 27: #include 
> is this supposed to be in brackets?
Whoops, not sure why I did that. Fixed.


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

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

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 5: Code-Review+2

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

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

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Jean-Daniel Cryans (Code Review)]

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

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


[java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

The testDisconnect test creates a single tablet table, scans the table,
disconnects a random tablet client, and then waits until the client has no
tablet clients. Since the Java client considers the master a tablet, this lasts
until the remaining tablet client times out. This commit changes the test to
instead disconnect all tablet clients, which shaves about 10 seconds off the
runtime.  There are also a few drive-by fixups, including renaming
AsyncKuduClient.getTableClients to getTabletClients.

The test fix exposed an existing race condition in the TabletClient
disconnect/reconnect handling which would cause an RPC to be 'lost' and never
be sent (eventually causing a timeout).

Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Reviewed-on: http://gerrit.cloudera.org:8080/3329
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/ITClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
6 files changed, 17 insertions(+), 25 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Adar Dembo,

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

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

to review the following change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..

[java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

The testDisconnect test creates a single tablet table, scans the table,
disconnects a random tablet client, and then waits until the client has no
tablet clients. Since the Java client considers the master a tablet, this lasts
until the remaining tablet client times out. This commit changes the test to
instead disconnect all tablet clients, which shaves about 10 seconds off the
runtime.  There are also a few drive-by fixups, including renaming
AsyncKuduClient.getTableClients to getTabletClients.

Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/ITClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestAsyncKuduSession.java
6 files changed, 14 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I00b2530c63d70b37cc7143de9f6190f0fbf349de
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans

[kudu-CR] [java-client] RPC timeout may sometimes be reported as max attemps violation

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] RPC timeout may sometimes be reported as max 
attemps violation
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/1763/

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

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

[kudu-CR] [java-client] disconnect all tablets in TestAsyncKuduSession.disconnectAndWait

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] disconnect all tablets in 
TestAsyncKuduSession.disconnectAndWait
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/1762/

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

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

[kudu-CR] [java-client] RPC timeout may sometimes be reported as max attempts violation

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] RPC timeout may sometimes be reported as max 
attempts violation
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/1764/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] RPC timeout may sometimes be reported as max attempts violation

[Dan Burkert (Code Review)]

Dan Burkert has uploaded a new patch set (#2).

Change subject: [java-client] RPC timeout may sometimes be reported as max 
attempts violation
..

[java-client] RPC timeout may sometimes be reported as max attempts violation

Fixes an issue where an RPC timeout could be reported in the error message as
too many attempts.

Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
1 file changed, 5 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] RPC timeout may sometimes be reported as max attempts violation

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java-client] RPC timeout may sometimes be reported as max 
attempts violation
..

[java-client] RPC timeout may sometimes be reported as max attempts violation

Fixes an issue where an RPC timeout could be reported in the error message as
too many attempts.

Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
1 file changed, 5 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c5676ab5bd05170505ef0313b919c5475ae6b37
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1469. Fix handling of fully-deduped requests after a leader change

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1469. Fix handling of fully-deduped requests after a 
leader change
..

KUDU-1469. Fix handling of fully-deduped requests after a leader change

This fixes KUDU-1469, a bug in which the leader and follower could get
into a tight loop of RPCs making no progress replicating operations.

The newly included test fails without the bug fix, and passes with it.
See its comments for details on the bug itself.

Change-Id: Iced21ae1b69c1079efc9aa9cf23e2fa592b8bebd
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/raft_consensus-itest.cc
8 files changed, 157 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iced21ae1b69c1079efc9aa9cf23e2fa592b8bebd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1469. Fix handling of fully-deduped requests after a leader change

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1469. Fix handling of fully-deduped requests after a 
leader change
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/1756/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iced21ae1b69c1079efc9aa9cf23e2fa592b8bebd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1473: fix some tablet lock usage in CatalogManager

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3309/2//COMMIT_MSG
Commit Message:

Line 15: GetTableLocations() call to a table whose tablets have been assigned 
but are
I'm not entirely following what GetTableLocations has to do with this bug - 
isn't the crash in ProcessPendingAssignments?


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

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

[kudu-CR] KUDU-1469. Fix handling of fully-deduped requests after a leader change

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1469. Fix handling of fully-deduped requests after a 
leader change
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/1757/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iced21ae1b69c1079efc9aa9cf23e2fa592b8bebd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1398 CFile index blocks can store shortest separating prefix

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1398 CFile index blocks can store shortest separating 
prefix
..


Patch Set 4:

(15 comments)

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

PS4, Line 17: compatibility with bloomfile
what's this mean?


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/binary_dict_block.cc
File src/kudu/cfile/binary_dict_block.cc:

Line 131: last_key_.assign_copy(last->data(), last->size());
instead of copying this every time, could we use GetLast on the code word 
builder, then look it up in our dictionary? What you have here will probably 
hurt write performance


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:

Line 132: Status BinaryPlainBlockBuilder::GetLastKey(void *key_void) const {
can we refactor this to share code with GetFirstKey? eg a GetKeyAtIndex(int 
idx)?


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/cfile_util.cc
File src/kudu/cfile/cfile_util.cc:

Line 126: void SeparatingKey(const faststring& left, const faststring& right, 
faststring *target) {
can you add some unit tests for this function? (eg the examples from the commit 
message)


Line 127:   DCHECK(memcmp(left.data(), right.data(), min(left.size(), 
right.size())) <=
I think DCHECK_LE(Slice(left).compare(Slice(right), 0); is probably more 
readable. Especially if you change this function to take slices as arguments


Line 130:   if (left.size() == 0) {
.empty()


Line 131: // special case for first block: use the whole key
not sure why this is the case


Line 135: target->assign_copy(right.data(), cpl == right.size() ? cpl : cpl 
+ 1);
hm, this is a slightly different algorithm than the one used in 
gutil/strings/util.h (FindShortestSeparator). eg their example:

  // FindShortestSeparator("foobar", "foxhunt", ) => sep == "fop"

you would return 'fox', right? Either one is the same length, but I guess 
theirs ends up tighter to the left bound whereas yours ends up somewhere in the 
middle. Can you think of any advantages/disadvantages between the two? As far 
as I can think, either is correct, but maybe I'm missing something.


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/cfile_util.h
File src/kudu/cfile/cfile_util.h:

Line 63:   bool deltafile;
I think I'd prefer this be called something like 'optimize_index_keys' or 
something, rather than specifically referring to whether it's a deltafile.

Another option is to have an int 'preserve_index_key_prefix_length' or 
somesuch, where for delta keys we pass the maximum possible encoded length of a 
delta key. This ensures that we can still decode it, but we might still be able 
to truncate the value (since the delta entries themselves can be arbitrarily 
long with user data)


Line 110: // Computes the shortest key satisfying left < key < right and places 
it into target.
should clarify whether 'target' may be the same faststring as 'left' or 'right' 
or not


Line 111: void SeparatingKey(const faststring& left, const faststring& right, 
faststring *target);
rename to GetSeparatingKey


PS4, Line 111: const faststring& left, const faststring& right
may make sense to use StringPiece or Slice here rather than specifically 
faststring


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

Line 141: last_key_.clear();
no need (it's initialized to clear)


Line 437:   SeparatingKey(last_key_, tmp_buf_, _buf);
maybe make SeparatingKey actually just shorten tmp_buf_ in place to avoid the 
extra allocation here


http://gerrit.cloudera.org:8080/#/c/3304/4/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:

Line 265: if (count_ > 0) {
usually we put the error case first


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I68ae9146fabd4a19b17d103d118d2d60e28bb315
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1466: improve error message when writes fail at TS

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: WIP: KUDU-1466: improve error message when writes fail at TS
..

WIP: KUDU-1466: improve error message when writes fail at TS

Currently, when we hit certain types of tablet server errors,
we fall back to re-requesting locations from the master. If
the timing of the errors lines up right, the last request to the
master may have a very short time out, in which case we will
misreport the write as failing due to a timeout on the
GetTableLocations() RPC, rather than due to the actual error on the
tablet server.

Injecting a bit of latency into GetTableLocations() reproduces the
issue reliably in ClientTest.TestFailedDnsResolution which is
already quite flaky in TSAN builds due to this issue.

This is a WIP patch as one potential way to solve it -- have the
location picker keep track of the "best" error seen so far. But,
perhaps it's actually better for this to be done in the retriable RPC.
Posting in order to get some comments on the best approach.

Change-Id: I5f1de8159e515cbb5f52fdc440d71370437c1af2
---
M src/kudu/client/client-test.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/rpc/retriable_rpc.h
4 files changed, 31 insertions(+), 5 deletions(-)


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

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

[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
..

[java client] fix reruns of TestKuduTable.testGetLocations

The test expected a table count including the table created in
testAlterTable, which meant that surefire reruns would always fail, as they
clean up the minicluster state (via @AfterClass) but only run the particular
failed test.

Why not generalize? My thought process went something like this:
1. Let's add an @After to BaseKuduTest that enumerates all tables and
   deletes them.
2. #1 is a tax on every test, and besides, if we're going to undo all
   destructive changes, we should also restart stopped processes.
3. If we're going to do #2, we may as well convert the existing @BeforeClass
   and @AfterClass into @Before and @After instead, since that's the same
   thing semantically but less code and guaranteed to capture every change.
4. But cluster setup in the Java tests is slow due to the Thread.sleep(300)
   performed by every started daemon. Let's do what we do in C++ tests and
   ask the servers to dump a file, sleeping until that file appears.
5. #4 is a lot of refactoring and doesn't address the multiple master case
   well. Let's punt on the whole endeavour and do a targeted fix instead.

Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
---
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
1 file changed, 30 insertions(+), 22 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1353: remove per-tablet replica locations cache

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1353: remove per-tablet replica locations cache
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/1760/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6376b5307f75f5d505b33a5ff4262da619cd1d8d
Gerrit-PatchSet: 9
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] KUDU-1473: fix some tablet lock usage in CatalogManager

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/1761/

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

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

[kudu-CR] [java client] fix reruns of TestKuduTable.testGetLocations

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java client] fix reruns of TestKuduTable.testGetLocations
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/1759/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44220d3c5ee1816f2bcf345c6062d33616145da2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1473: fix some tablet lock usage in CatalogManager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1473: fix some tablet lock usage in CatalogManager
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3309/2//COMMIT_MSG
Commit Message:

Line 15: GetTableLocations() call to a table whose tablets have been assigned 
but are
> I'm not entirely following what GetTableLocations has to do with this bug -
Yes, the crash is in ProcessPendingAssignments, but the DCHECK itself is:

  DCHECK(lock_.HasReaders() || lock_.HasWriteLock());

So we accessed an object without a lock while someone else had that object 
locked for reading or for writing. I don't think that "someone else" is the 
background thread, so I think it's got to be another client thread. A 
GetTableLocations() or GetTabletLocations() on the same table would lock some 
tablets for reading, and since the Java code in question does exactly that 
(create a table followed by two calls to GetTabletLocations() on the same 
table), it's conceivable that it's responsible.

It's strange, though, since the Java client is supposed to wait for the table 
to be created before continuing. That's why I'm going to spend some more time 
digging into this.


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

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