[kudu-CR] WIP: Integration test for replay cache

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Integration test for replay cache
..


Patch Set 5:

thanks for taking a look. I can certainly spend some time elaborating on the 
invariants. Regarding the one you mention yes, that is correct, there's a check 
to make sure that's true.

weird that it doesn't trigger elections for you. my logs are full of elections, 
for instance I just did one run that got to term 500

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: Integration test for replay cache

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: Integration test for replay cache
..


Patch Set 5:

Been looking at the new test and the code for the last couple hours.

A couple concerns I have from running the test:
- at least on my system, it doesn't trigger any leader elections. Perhaps we 
need a thread which is randomly cycling through the tservers and stopping them? 
Or explicitly call StartElection to force term bumps and re-elections?
- I ran it through coverage, and perhaps due to the above, there are some lines 
in ResultTracker that aren't covered. For example, in 
RecordCompletionAndRespond:

24820:  225:if (MustHandleRpc(handler_attempt_no, completion_record, 
ongoing_rpc)) {
12410:  226:  if (ongoing_rpc.context != nullptr) {
11873:  227:if (PREDICT_FALSE(ongoing_rpc.response != response)) {
 3746:  228:  
ongoing_rpc.response->CopyFrom(*completion_record->response);
 1873:  229:}
11873:  230:LogAndTraceAndRespondSuccess(ongoing_rpc.context, 
*ongoing_rpc.response);
11873:  231:  }
24820:  232:  orpc_iter = 
completion_record->ongoing_rpcs.erase(orpc_iter);
12410:  233:} else {
#:  234:  ++orpc_iter;
-:  235:}
-:  236:  }
3:  237:}


( indicates a non-covered line). Not sure yet whether this line is actually 
impossible to hit, or just not hit due to a test deficiency.

Will keep reading through this trying to fully understand what's going on. To 
aid that, one suggestion is to do another pass over the ResultTracker API and 
explain some more of the invariants. For example: if one thread uses 
ChangeDriver, what requirements does that impose on the previous driver? (I 
think it means that the previous driver must not call RecordSuccess, and it's 
up to the caller to ensure that the previous driver can't succeed if a new 
driver steals it?)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Update docs on how to run gcovr

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Update docs on how to run gcovr
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3508/1/README.adoc
File README.adoc:

PS1, Line 221: gcc (not clang)
this part is also incorrect (my fault for missing it originally)


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

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

[kudu-CR] WIP: Integration test for replay cache

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Integration test for replay cache
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] WIP: Integration test for replay cache

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Integration test for replay cache
..

WIP: Integration test for replay cache

This adds a new integration test for replay cache that writes to all
replicas of a tablet at the same time. This uses the fact that all replicas
must eventually agree, and once the result is stored in replay cache,
followers can also reply to rpc's, to store all the responses obtained from
all the replicas, for the same rpcs. This then proceeds to make sure the
responses are the same.

WIP because it needs cleanup and pulling of some suff to other patches.

Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_service.cc
7 files changed, 271 insertions(+), 76 deletions(-)


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

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

[kudu-CR] WIP: Integration test for replay cache

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Integration test for replay cache
..

WIP: Integration test for replay cache

This adds a new integration test for replay cache that writes to all
replicas of a tablet at the same time. This uses the fact that all replicas
must eventually agree, and once the result is stored in replay cache,
followers can also reply to rpc's, to store all the responses obtained from
all the replicas, for the same rpcs. This then proceeds to make sure the
responses are the same.

WIP because it needs cleanup and pulling of some suff to other patches.

Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_service.cc
7 files changed, 270 insertions(+), 77 deletions(-)


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

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

[kudu-CR] WIP: Integration test for replay cache

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Integration test for replay cache
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Jean-Daniel Cryans (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java client] RPCs can get lost in a TabletClient race
..

[java client] RPCs can get lost in a TabletClient race

We saw hangs after running ITBLL for hours. Turns out that the recent
fixes in TabletClient introduced a new race condition. rpcs_inflight
is being cleaned in cleanup() by copying all the elements from it
and then calling clear(). Even though this is done under a lock, that
lock isn't protecting rpcs_inflight so it's possible to clear() rpcs
that were not copied out.

I haven't been able to recreate this race in unit tests, but it fixed
ITBLL.

Change-Id: Iaff89eb832d0d6f0dede198661856fae1a8585a0
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 20 insertions(+), 11 deletions(-)


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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 3:

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

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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3541/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:

PS1, Line 187: We got disconnected, cleanup ran, then we added the rpc to 
rpcs_inflight, so we need to
 :   // manually failOrRetry it.
> I think this is too vague. Multiple threads are at work here, right? It's n
Done


PS1, Line 678: // Removing from the iterator because ConcurrentHashMap doesn't 
have a way to retrieve and
 :   // remove all the entries atomically.
 :   for (Iterator iterator = 
rpcs_inflight.values().iterator(); iterator.hasNext();) {
 : KuduRpc rpc = iterator.next();
 : rpcs.add(rpc);
 : iterator.remove();
 :   }
> I don't get it; how is this any more "atomic" than the previous code? What 
It's not more atomic, we get around this by assuming we might be missing some 
rpcs that got added while iterating, hence the check in sendRpc above.

I removed the comment, it's not really making things clearer.


Line 685:   // After this, rpcs_inflight might still have entries since it 
could have been added
> Nit: it -> they
Done


Line 686:   // concurrently, and those RPCs will be handled by their called 
in sendRpc.
> Nit: "their called"?
Done


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

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

[kudu-CR] master failover-itest: eliminate some flakiness

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: master_failover-itest: eliminate some flakiness
..


Patch Set 1:

(1 comment)

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

Line 17: 1. Beyond basic EO semantics, RPC results would need to be persisted 
and
> For writes, we make sure that writes are executed exactly once, and because
Consider the sequence of events I described in the patch, which I'll reprint 
below:

  1. Pause() is issued.
  2. CreateTable() is issued.
  3. Leader master receives CreateTable() and creates the table.
  4. Leader master is paused before the CreateTable() response is sent.
  5. Client times out, finds the new master, and retries CreateTable().
  6. The retry fails because the table was already created in step 3.

The desired semantics are for the client to receive the exact same RPC response 
at the end of step 5 as it would have at the end of step 3 had the old leader 
master not been paused. But, given that in step 5 the client is talking to the 
new leader master, it seems to me that the only way to guarantee the exact RPC 
response is for the old leader master to have replicated it to its followers. 
Otherwise, how would the new leader master know how to build that RPC response?


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

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

[kudu-CR] master failover-itest: eliminate some flakiness

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: master_failover-itest: eliminate some flakiness
..


Patch Set 1:

(1 comment)

+2 with the commit message comment caveat

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

Line 17: 1. Beyond basic EO semantics, RPC results would need to be persisted 
and
For writes, we make sure that writes are executed exactly once, and because of 
that when a node rebuilds the response, we're sure that it must be the same 
across all nodes

why would we need to persist/replicate here?


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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 1:

(4 comments)

What releases (if any) are affected by this?

We've been applying bandaid after bandaid for these concurrency issues, and the 
lack of regression tests is increasingly concerning. Is there something missing 
from the client that'd make writing such tests easier?

http://gerrit.cloudera.org:8080/#/c/3541/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:

PS1, Line 187: We got disconnected, cleanup ran, then we added the rpc to 
rpcs_inflight, so we need to
 :   // manually failOrRetry it.
I think this is too vague. Multiple threads are at work here, right? It's not 
clear what the race is from this terse description. Can you rewrite with a 
concrete example?


PS1, Line 678: // Removing from the iterator because ConcurrentHashMap doesn't 
have a way to retrieve and
 :   // remove all the entries atomically.
 :   for (Iterator iterator = 
rpcs_inflight.values().iterator(); iterator.hasNext();) {
 : KuduRpc rpc = iterator.next();
 : rpcs.add(rpc);
 : iterator.remove();
 :   }
I don't get it; how is this any more "atomic" than the previous code? What am I 
missing?


Line 685:   // After this, rpcs_inflight might still have entries since it 
could have been added
Nit: it -> they


Line 686:   // concurrently, and those RPCs will be handled by their called 
in sendRpc.
Nit: "their called"?

Maybe just rewrite this paragraph to make it more clear.


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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 2:

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

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

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

[kudu-CR] WIP: Integration test for replay cache

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Integration test for replay cache
..

WIP: Integration test for replay cache

This adds a new integration test for replay cache that writes to all
replicas of a tablet at the same time. This uses the fact that all replicas
must eventually agree, and once the result is stored in replay cache,
followers can also reply to rpc's, to store all the responses obtained from
all the replicas, for the same rpcs. This then proceeds to make sure the
responses are the same.

WIP because it needs cleanup and pulling of some suff to other patches.

Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_service.cc
7 files changed, 267 insertions(+), 77 deletions(-)


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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3541/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java:

Line 686:   // concurrently, and those RPCs will be handled by their called 
in sendRpc.
I think "handled by their called in sendRpc" is a typo


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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java client] RPCs can get lost in a TabletClient race
..


Patch Set 1:

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

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

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

[kudu-CR] [java client] RPCs can get lost in a TabletClient race

[Jean-Daniel Cryans (Code Review)]

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

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

Change subject: [java client] RPCs can get lost in a TabletClient race
..

[java client] RPCs can get lost in a TabletClient race

We saw hangs after running ITBLL for hours. Turns out that the recent
fixes in TabletClient introduced a new race condition. rpcs_inflight
is being cleaned in cleanup() by copying all the elements from it
and then calling clear(). Even though this is done under a lock, that
lock isn't protecting rpcs_inflight so it's possible to clear() rpcs
that were not copied out.

I haven't been able to recreate this race in unit tests, but it fixed
ITBLL.

Change-Id: Iaff89eb832d0d6f0dede198661856fae1a8585a0
---
M java/kudu-client/src/main/java/org/kududb/client/TabletClient.java
1 file changed, 20 insertions(+), 11 deletions(-)


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

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

[kudu-CR] KUDU-1490: Show software version of tablet servers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1490: Show software version of tablet servers
..


Patch Set 5: Code-Review+2

Looks good!

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

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

[kudu-CR] KUDU-1490: Show software version of tablet servers

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1490: Show software version of tablet servers
..


Patch Set 5:

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

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

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

[kudu-CR] KUDU-1490: Show software version of tablet servers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1490: Show software version of tablet servers
..

KUDU-1490: Show software version of tablet servers

In the web UI, it could be helpful to know what version of the
software a tablet server is running. This commit addresses this
by adding a field to the initial registration heartbeat of a
tablet. The version is displayed in the Registration column of
the Tablet Servers page of the master web server.

See:
https://raw.githubusercontent.com/anjuwong/kudu/tserver-vers/tserver_vers_on_master_ui.png

Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/tserver/heartbeater.cc
3 files changed, 9 insertions(+), 2 deletions(-)


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

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

[kudu-CR] Show software version of tablet servers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Show software version of tablet servers
..


Patch Set 4:

(1 comment)

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

PS4, Line 7: Show software version of tablet servers
   : 
   : KUDU-1490: displaying tserver versions in master web UI
Sorry, I should have mentioned this earlier: these two lines should be 
combined. What Todd was trying to explain is that we use the following template 
for our commit messages.

  

  


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Show software version of tablet servers

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Show software version of tablet servers
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Show software version of tablet servers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Show software version of tablet servers
..

Show software version of tablet servers

KUDU-1490: displaying tserver versions in master web UI

In the web UI, it could be helpful to know what version of the
software a tablet server is running. This commit addresses this
by adding a field to the initial registration heartbeat of a
tablet. The version is displayed in the Registration column of
the Tablet Servers page of the master web server.

See:
https://raw.githubusercontent.com/anjuwong/kudu/tserver-vers/tserver_vers_on_master_ui.png

Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/tserver/heartbeater.cc
3 files changed, 9 insertions(+), 2 deletions(-)


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

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

[kudu-CR] [java-client] refactor AsyncKuduSession

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 563: doFlush(fullBuffer);
> Not sure, but I don't think doFlush can throw.  In general, methods that re
I think doFlush() may throw (unchecked excpetions) if the buffer passed to it 
was empty, in which case it'd callback() on the global flushNotification 
deferred, which would potentially run user code that would throw.

It doesn't seem like that could happen from here (where fullBuffer is, well, 
full) but I wonder what that means for other doFlush() callers.


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Batch.java
File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 46:   final List operations = new ArrayList<>(125);
> nothing, and in fact it should probably be more like 125 (default buffer si
So why bother predicting it at all?

Anyway, if you do retain the preallocation, please add a comment explaining why 
125 was chosen.


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

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

[kudu-CR] Show software version of tablet servers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Show software version of tablet servers
..


Patch Set 3:

(1 comment)

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

PS2, Line 7: software version of tabl
> Done
Nit: you can drop the word "Addressing"; it's implied.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Show software version of tablet servers

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: Show software version of tablet servers
..


Patch Set 3:

(3 comments)

> (2 comments)
 > 
 > please also add a new assertion in registration-test.cc to check
 > that the version info string shows up in the tablet servers page

Working on that now

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

PS2, Line 7: software version of tabl
> "software version of tablet servers"
Done


http://gerrit.cloudera.org:8080/#/c/3535/2/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

Line 277:   reg->set_software_version(VersionInfo::GetShortVersionString());
> Nit: the comment isn't quite correct in that registration information may b
Done


Line 279:   return Status::OK();
> I think better to just combine the above two lines into one rather than add
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Show software version of tablet servers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Show software version of tablet servers
..

Show software version of tablet servers

Addressing KUDU-1490: displaying tserver versions in master
web UI.

In the web UI, it could be helpful to know what version of the
software a tablet server is running. This commit addresses this
by adding a field to the initial registration heartbeat of a
tablet. The version is displayed in the Registration column of
the Tablet Servers page of the master web server.

See:
https://raw.githubusercontent.com/anjuwong/kudu/tserver-vers/tserver_vers_on_master_ui.png

Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
---
M src/kudu/master/master.proto
M src/kudu/tserver/heartbeater.cc
2 files changed, 4 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Show software version of tablet servers

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Show software version of tablet servers
..


Patch Set 3:

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

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

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

[kudu-CR] Show software vers to tablets in master web server

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Show software vers to tablets in master web server
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3535/2/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

Line 277:   // Also register the Kudu version in the initial heartbeat
Nit: the comment isn't quite correct in that registration information may be 
sent outside of the initial heartbeat too. Given the simplicity of the new code 
to follow, I'd just remove the comment altogether.


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

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

[kudu-CR] Disable core dumping in integration tests which crash

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Disable core dumping in integration tests which crash
..


Patch Set 2: -Verified

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

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

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

[kudu-CR] Remove unused RunShellProcess() function

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Remove unused RunShellProcess() function
..


Remove unused RunShellProcess() function

Change-Id: I881410660a360d2c6f75046dc95d61453b562bd2
Reviewed-on: http://gerrit.cloudera.org:8080/3533
Reviewed-by: Mike Percy 
Tested-by: Kudu Jenkins
---
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
2 files changed, 0 insertions(+), 33 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I881410660a360d2c6f75046dc95d61453b562bd2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Show software vers to tablets in master web server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Show software vers to tablets in master web server
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Show software vers to tablets in master web server

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new patch set (#2).

Change subject: Show software vers to tablets in master web server
..

Show software vers to tablets in master web server

In the web UI, it could be helpful to know what version of the
software a tablet server is running. This commit addresses this
by adding a field to the initial registration heartbeat of a
tablet. The version is displayed in the Registration column of
the Tablet Servers page of the master web server.

See:
https://raw.githubusercontent.com/anjuwong/kudu/tserver-vers/tserver_vers_on_master_ui.png

Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
---
M src/kudu/master/master.proto
M src/kudu/tserver/heartbeater.cc
2 files changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Disable core dumping in integration tests which crash

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Disable core dumping in integration tests which crash
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2eca1395fa3438861ee1b1ff49978408fed390f4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Remove unused RunShellProcess() function

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Remove unused RunShellProcess() function
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] Remove unused RunShellProcess() function

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Remove unused RunShellProcess() function
..


Patch Set 2:

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

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

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

[kudu-CR] Remove unused RunShellProcess() function

[Todd Lipcon (Code Review)]

Hello Mike Percy,

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

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

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

Change subject: Remove unused RunShellProcess() function
..

Remove unused RunShellProcess() function

Change-Id: I881410660a360d2c6f75046dc95d61453b562bd2
---
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
2 files changed, 0 insertions(+), 33 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I881410660a360d2c6f75046dc95d61453b562bd2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Remove unused RunShellCommand() function

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Remove unused RunShellCommand() function
..


Patch Set 1: Code-Review+2

(1 comment)

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

Line 7: Remove unused RunShellCommand() function
Nit: It's named RunShellProcess()


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

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

[kudu-CR] Show software vers to tablets in master web server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Show software vers to tablets in master web server
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84b32608c9e12a47f422b5047591d4098d187478
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Remove unused RunShellCommand() function

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Remove unused RunShellCommand() function
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I881410660a360d2c6f75046dc95d61453b562bd2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Disable core dumping in integration tests which crash

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Disable core dumping in integration tests which crash
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2eca1395fa3438861ee1b1ff49978408fed390f4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4: -Verified

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
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] Implement SchemaRelationProvider

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Implement SchemaRelationProvider
..


Patch Set 1:

(1 comment)

LGTM except for the style nit.

http://gerrit.cloudera.org:8080/#/c/3529/1/java/kudu-spark/src/main/scala/org/kududb/spark/kudu/DefaultSource.scala
File java/kudu-spark/src/main/scala/org/kududb/spark/kudu/DefaultSource.scala:

Line 140:   override def schema: StructType = {
I think the body of this would be clearer as:

userSchema match {
  case Some(x) =>
StructType(x.fields.map(uf => table.getSchema.getColumn(uf.name))
   .map(kuduColumnToSparkField))
  case None =>

StructType(table.getSchema.getColumns.asScala.map(kuduColumnToSparkField).toArray)
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4194d7ec139f0f9e3c1d508e8aca860484839d56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andy Grove 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] build: adjust boost location check slightly

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: build: adjust boost location check slightly
..


build: adjust boost location check slightly

Commit 5b83695 broke the Cloudera packaging builds because the boost prefix
check interpreted regular expression characters in the directory names [1].
This patch fixes that by replacing the regular expression matching with a
simpler substring search.

1. 
/data/jenkins/workspace/generic-package-debian64-8-0/Kudu-Packaging-Kudu-2016-06-29_09-15-06/kudu-1.0.0+cdh5.4.0+0-1.kudu1.0.0.p0.522~jessie

Change-Id: Iac7ba5b803d04736d9ed0e1f9dc5093bef1051da
Reviewed-on: http://gerrit.cloudera.org:8080/3530
Reviewed-by: Dan Burkert 
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
1 file changed, 5 insertions(+), 2 deletions(-)

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



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

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

[kudu-CR] build: adjust boost location check slightly

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: build: adjust boost location check slightly
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iac7ba5b803d04736d9ed0e1f9dc5093bef1051da
Gerrit-PatchSet: 1
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] build: adjust boost location check slightly

[Adar Dembo (Code Review)]

Hello Dan Burkert,

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

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

to review the following change.

Change subject: build: adjust boost location check slightly
..

build: adjust boost location check slightly

Commit 5b83695 broke the Cloudera packaging builds because the boost prefix
check interpreted regular expression characters in the directory names [1].
This patch fixes that by replacing the regular expression matching with a
simpler substring search.

1. 
/data/jenkins/workspace/generic-package-debian64-8-0/Kudu-Packaging-Kudu-2016-06-29_09-15-06/kudu-1.0.0+cdh5.4.0+0-1.kudu1.0.0.p0.522~jessie

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


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

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

[kudu-CR] build: adjust boost location check slightly

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: build: adjust boost location check slightly
..


Patch Set 1:

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

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

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

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 4: -Verified

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

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

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