[kudu-CR] KUDU-1467. Fix incorrect bootstrap replay issue with UPSERT

[Todd Lipcon (Code Review)]

Hello David Ribeiro Alves,

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

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

to review the following change.

Change subject: KUDU-1467. Fix incorrect bootstrap replay issue with UPSERT
..

KUDU-1467. Fix incorrect bootstrap replay issue with UPSERT

This fixes an issue with UPSERT that was causing fuzz-itest to be ~5% flaky.
The issue turned out to be essentially the same as KUDU-1341 (fixed in
1ff209e85b4c2cc4beda7560d328b7ed05e008d2) except pertaining to UPSERT
operations which acted as mutations, whereas the original bug was
for direct UPDATE operations.

In addition to the existing fuzz test which exposes this bug, this patch
includes a specific minimal fuzz sequence which triggered the bug
deterministically.

To verify that the bug is fixed, I looped fuzz-itest 500 times:
http://dist-test.cloudera.org/job?job_id=todd.1464152165.32007

Change-Id: Id443e171bc80833737486a96a3fd6fcce7a2363b
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
3 files changed, 26 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id443e171bc80833737486a96a3fd6fcce7a2363b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 

[kudu-CR] Remove default table partitioning

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Remove default table partitioning
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3131/9/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:

Line 24
Nit: don't unroll.


http://gerrit.cloudera.org:8080/#/c/3131/9/java/kudu-spark/src/main/scala/org/kududb/spark/kudu/KuduContext.scala
File java/kudu-spark/src/main/scala/org/kududb/spark/kudu/KuduContext.scala:

Line 20: import java.util
If you're not changing file contents, could you avoid changing the import order?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7021d7950f8dbb4918503ea6fab2e6ee35076064
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1267: Calling getString() on RowResult for wrong type gave weird exception

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1267: Calling getString() on RowResult for wrong type gave 
weird exception
..


Patch Set 1:

(4 comments)

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

Line 568: }
> What happened here?
I think you missed this as well.


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

Line 152: //Do negative testing on string type
> Nit: // Do negative...
I think you missed this one: please add an empty space between "//" and "Do".


Line 154: if (scanner.hasMoreRows()) {
> If this is false, we won't actually run the remainder of the test. Shouldn'
You didn't address this one either.


Line 157:   boolean exceptionThrown = false;
:   try {
: next.getInt("c2");
:   } catch (ClassCastException e) {
: exceptionThrown = true;
:   }
:   assertTrue("ClassCastException was not thrown when accessing a 
string column with getInt", exceptionThrown);
: }
> Here's a simpler way to do it:
Nor this one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idc8c6e5e382ca9f6280072f59acf4257fd13fc6b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ted Malaska 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3190/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 40:   lock_guard l(_);
> no, the lock protects access to clients_ and to the ClientState that belong
I suspect it will be, as nearly every RPC will call CheckDuplicate(), no?


Line 78:   LOG(FATAL) << "Wrong state";
> the whole point is that we only call this once per RPC, if the caller got N
OK, could we at least log what the wrong state was?


Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
:   CompletionRecord* completion_record = 
FindOrDie(client_state->completion_records,
:   sequence_number);
> why? if we are responding to an RPC that is not being tracked then it's an 
Well, this goes back to the lack of documentation thing. :)

You're designing a module with "hard" boundaries. That is, if one of us misuses 
the ResultTracker, it crashes.

Without context and in isolation, I tend to assume new modules should have 
"soft" boundaries, where misuse leads to returned errors. And to be fair, there 
are advantages to soft boundaries: it's easier to test such a module in 
isolation.

Anyway, there's nothing inherently wrong with hard boundaries, you just need to 
be very clear in the interface regarding what happens in the event of misuse.


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

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

[kudu-CR] Add a ResultTracker class that will track server side results

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3190/6/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 45: // of the RPC at that the corresponding server function should be 
executed. If any other value is
Nit: "at that the" --> "and that the"


Line 49: // client appropriately,
Nit: trailing comma here?


Line 55: // If, on the other hand, if execution of the server function is not 
successful then one of
Nit: too many 'ifs' here.


Line 96:   RpcState CheckDuplicate(const std::string& client_id,
I'd rename the function slightly to imply that it's not just a stateless 
"check". That is, it'll actually register the caller's input internally such 
that RecordCompletionAndRespond() does something with it.


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

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

[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Integrate the ResultTracker into the rpc subsystem and add a 
test
..


Patch Set 6:

(9 comments)

It's becoming increasingly clear to me that Todd needs to take a deep look at 
this series; there's too much here that I don't know.

http://gerrit.cloudera.org:8080/#/c/3192/6//COMMIT_MSG
Commit Message:

Line 10: to specify a new mehtod option when defining rpc service methods.
Nit: method


http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/common/common.proto
File src/kudu/common/common.proto:

Line 313: // An option for RPC methods that allows to set whether that method's
Why define this here and not in rpc_header.proto? Wouldn't doing that mean 
proto-gen-krpc won't depend on kudu_common?


http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:

Line 46: #include "kudu/common/common.pb.h"
Nit: should come before gutil.


Line 172: 
Nit: extra line?


http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/rpc_context.h
File src/kudu/rpc/rpc_context.h:

Line 178:   bool AreResultsTracked() const { return result_tracker_.get() != 
nullptr; }
I don't think this is necessary given the below method, which is very easy to 
use to test for the presence of a tracker.


Line 180:   const scoped_refptr GetResultTracker() const {
Shouldn't this return a cref and return the result_tracker_ as-is, instead of 
calling get()?

Also, call it result_tracker()?


http://gerrit.cloudera.org:8080/#/c/3192/6/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

Line 96: if (!call->header().has_request_id()) {
   : ctx = new RpcContext(call, req.get(), resp, nullptr);
   :   } else {
   : ctx = new RpcContext(call, req.get(), resp, 
method_info->result_tracker);
   :   }
Convert into a ternary:

  RpcContext* ctx = new RpcContext(call, req.get(), resp, 
call->header().has_request_id() ? method_info->result_tracker : nullptr);


Line 103: ResultTracker::RpcState state = 
ctx->GetResultTracker()->CheckDuplicate(
It's interesting that 'req' isn't considered at all in CheckDuplicate(). We're 
relying entirely on seq_no() instead, I believe.


Line 116: LOG(FATAL) << "Unknown state.";
Log the state too.


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

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

[kudu-CR] Add request id and sequence number to the rpc request header

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Add request id and sequence number to the rpc request header
..


Add request id and sequence number to the rpc request header

This adds a new (optional) field to RpcHeader: RequestIdPB.

This new field contains the information necessary to implement the "replay 
cache"
component, including the unique id of the client and the rpc's sequence number.
RequestIdPB also includes two additional "helper" fields, one to keep track of
number of attempts of a particular rpc, so that we can skip the replay cache
altogether in certain cases and one to let the server know that it can garbage
collect completion records.

Change-Id: I30b07f070f8e80aaad982ccfe71ffb7e22a6a703
Reviewed-on: http://gerrit.cloudera.org:8080/3079
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M src/kudu/rpc/rpc_header.proto
1 file changed, 22 insertions(+), 0 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I30b07f070f8e80aaad982ccfe71ffb7e22a6a703
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Move the PbTracer in rpc_context.cc to pb_util

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Move the PbTracer in rpc_context.cc to pb_util
..


Move the PbTracer in rpc_context.cc to pb_util

We'll need to use this outside of RpcContext so this moves
it to a place where it makes more sense and can be used
elsewhere.

Change-Id: Ib601deea9fe0dcc6e2428416a6993c7567ff7335
Reviewed-on: http://gerrit.cloudera.org:8080/3194
Tested-by: David Ribeiro Alves 
Reviewed-by: Adar Dembo 
---
M src/kudu/rpc/rpc_context.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
3 files changed, 54 insertions(+), 36 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib601deea9fe0dcc6e2428416a6993c7567ff7335
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] java: use truncated randomized exponential backoff for retries

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: java: use truncated randomized exponential backoff for retries
..


Patch Set 1: Code-Review+2

(1 comment)

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

Line 924: // Randomized exponential backoff, truncated at 4096ms.
> The Java client uses this somewhat indiscriminately including things like w
> Looking at the C++ client, there are a number of different strategies 
> sprinkled around.

Yeah, and it's made me rather grumpy. Makes thinking about backoff a very 
unpredictable affair.

Anyway, I guess this isn't any worse, since the Java client did its own thing 
previously too.


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

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

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will addressing the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear waht it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
2 files changed, 347 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Fix typo in RPC design doc

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Fix typo in RPC design doc
..


Patch Set 1: Code-Review+2

Test failures are from the change this is based on. Rebase and you should be 
fine.

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

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

[kudu-CR] service_pool: only log queue overflows once per second

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: service_pool: only log queue overflows once per second
..


Patch Set 1: Code-Review+2

Looks good for branch-0.9.x too.

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

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

[kudu-CR] Integrate the ResultTracker into the rpc subsystem and add a test

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Integrate the ResultTracker into the rpc subsystem and add a 
test
..

Integrate the ResultTracker into the rpc subsystem and add a test

This integrates the ResultTracker into the rpc subsystem by allowing
to specify a new mehtod option when defining rpc service methods.
When this method option is specificed _and_ the call's rpc header
has a RequestIdPB the results for the call will be tracked and the
call may have exactly once semantics.

This allows to have the simplest form of exactly once semantics for
single server calls. Of course limitations apply, like response
persistence across restarts is not supported, neither is propagating/rebuilding
responses to/on replicas for replicated calls. If support for this
is needed it will have to be done ad-hoc, case by case.

Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
---
M src/kudu/common/common.proto
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/protoc-gen-krpc.cc
A src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
11 files changed, 321 insertions(+), 36 deletions(-)


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

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

[kudu-CR] Add a RpcContext::RespondFailure() method

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a RpcContext::RespondFailure() method
..

Add a RpcContext::RespondFailure() method

In the case of certain errors we use RpcContext::RespondSuccess() because
setting the error is call dependent and it was done on the call's request.
Since we'll be relying on the RpcContext to call the appropriate method
on the ResultTracker, on the common case, we'd still be marking these error
cases as successful RPCs whose results we keep. This is problematic for
various reasons, the main one being that if a client gets a transient error
back (e.g. the call was throttled) all subsequent attemtpts at the same
call will get the same result.

To avoid this we need to be able to flag that a call failed even if we don't
need anything else done to the response. To this goal this patch adds
RpcContext::RespondFailure(), which currently does nothing else besides
call RespondSuccess(), but will do so when the result tracker is integrated
in.

Change-Id: I9f1387ba0f837046a8056e77f73a3982f06c73a2
---
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
2 files changed, 16 insertions(+), 0 deletions(-)


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

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

[kudu-CR] java: use truncated randomized exponential backoff for retries

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: java: use truncated randomized exponential backoff for retries
..


Patch Set 1:

(1 comment)

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

Line 924: // Randomized exponential backoff, truncated at 4096ms.
Why not use the same backoff scheme as the C++ client? I think that's linear 
backoff, 1 ms per attempt + 0-4 additional ms.


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

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

[kudu-CR] Bump the master branch's version to 1.0.0-SNAPSHOT

[Jean-Daniel Cryans (Code Review)]

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

Change subject: Bump the master branch's version to 1.0.0-SNAPSHOT
..


Bump the master branch's version to 1.0.0-SNAPSHOT

Change-Id: I9d4815052072be6e7a163da92f5609634d44b4fc
Reviewed-on: http://gerrit.cloudera.org:8080/3178
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M java/interface-annotations/pom.xml
M java/kudu-client-tools/pom.xml
M java/kudu-client/pom.xml
M java/kudu-csd/pom.xml
M java/kudu-flume-sink/pom.xml
M java/kudu-mapreduce/pom.xml
M java/kudu-spark/pom.xml
M java/pom.xml
M version.txt
9 files changed, 10 insertions(+), 10 deletions(-)

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



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

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

[kudu-CR] Add request id and sequence number to the rpc request header

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add request id and sequence number to the rpc request header
..


Patch Set 5: Code-Review+2

Looks good to me, leaving open for Todd.

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

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

[kudu-CR] java: use truncated randomized exponential backoff for retries

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: java: use truncated randomized exponential backoff for retries
..


Patch Set 1:

(1 comment)

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

Line 924: // Randomized exponential backoff, truncated at 4096ms.
> Why not use the same backoff scheme as the C++ client? I think that's linea
The Java client uses this somewhat indiscriminately including things like 
waiting for a table to be created, etc. Exponential backoff seems like a more 
reasonable strategy than linear.

Looking at the C++ client, there are a number of different strategies sprinkled 
around. For scanners, we use:

// Exponential backoff with jitter anchored between 10ms and 20ms, and an
// upper bound between 2.5s and 5s.
MonoDelta sleep = MonoDelta::FromMilliseconds(
(10 + rand() % 10) * static_cast(std::pow(2.0, std::min(8, 
scan_attempts_ - 1;

so this is approximately the same, except starting at 10ms instead of 1ms


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

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

[kudu-CR] Move the MetaCacheServerPicker into meta_cache.h/cc

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Move the MetaCacheServerPicker into meta_cache.h/cc
..


Patch Set 15: Code-Review+2

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

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

[kudu-CR] Add a generic retriable rpc class

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a generic retriable rpc class
..


Patch Set 12: Code-Review+2

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

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

[kudu-CR] Add unique id generation to the client

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add unique id generation to the client
..


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3077/11/src/kudu/client/client_builder-internal.cc
File src/kudu/client/client_builder-internal.cc:

Line 26: default_rpc_timeout_(MonoDelta::FromSeconds(10)) {}
Can you revert this change? It's just unnecessary noise.


http://gerrit.cloudera.org:8080/#/c/3077/11/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:

Line 42: namespace client {
Hmm, while the test is called client-stress-test, it's part of the 
integration-tests module, so I don't know if it should be in the client 
namespace (which I'd expect for things in the client module). Why did you need 
to make this change?


Line 49: class ClientStressTest: public KuduTest {
This syntax is quite a bit more rare in the Kudu codebase than the original 
syntax. Could you revert it? Below too.


Line 279:  << total_num_rejections << " memory rejections";
The old indentation was nicer, I think.


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

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

[kudu-CR] Add a request tracker to track client rpc sequence numbers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a request tracker to track client rpc sequence numbers
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3078/7/src/kudu/rpc/request_tracker.h
File src/kudu/rpc/request_tracker.h:

Line 43: // this allows to get the lowest incomplete RPC. When this information 
is sent
Nit: instead of "this allows..." say "we can determine the first incomplete 
RPC."


Line 44: // to the server it can use it to garbage collect rpc results that it 
might be
Nit: RPC, not rpc


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

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

[kudu-CR] Add a RpcContext::RespondFailure() method

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a RpcContext::RespondFailure() method
..

Add a RpcContext::RespondFailure() method

In the case of certain errors we use RpcContext::RespondSuccess() because
setting the error is call dependent and it was done on the call's request.
Since we'll be relying on the RpcContext to call the appropriate method
on the ResultTracker, on the common case, we'd still be marking these error
cases as successful RPCs whose results we keep. This is problematic for
various reasons, the main one being that if a client gets a transient error
back (e.g. the call was throttled) all subsequent attemtpts at the same
call will get the same result.

To avoid this we need to be able to flag that a call failed even if we don't
need anything else done to the response. To this goal this patch adds
RpcContext::RespondFailure(), which currently does nothing else besides
call RespondSuccess(), but will do so when the result tracker is integrated
in.

Change-Id: I9f1387ba0f837046a8056e77f73a3982f06c73a2
---
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
2 files changed, 16 insertions(+), 0 deletions(-)


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

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

[kudu-CR] Non-covering Range Partitions design doc

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Non-covering Range Partitions design doc
..


Patch Set 10: Code-Review+2

Seems fine to me. Not sure what other feedback you're looking to collect, or 
whether the design is good to go.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3e530eda60c00faf066c41b6bdb2b37f6d96a5dc
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpcz: break out trace metrics into separate PB fields

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: rpcz: break out trace metrics into separate PB fields
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] Change flush defaults to encourage parallel IO and larger flushes

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Change flush defaults to encourage parallel IO and larger 
flushes
..


Patch Set 2:

(2 comments)

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

Line 48: // The default value is optimized for throughput in the case that
There's a similar comment in block_manager.cc; could you update that too? I 
think block_coalesce_close=true is uninteresting given 
cfile_do_on_finish=flush, but I'll leave that to your judgment.


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

Line 32: DEFINE_int32(flush_threshold_mb, 1000,
Nit: 1024 (power of 2)?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1c70d9c76ed33bbfca5480e1d1f343c6dab36d3b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix typo in RPC design doc

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Fix typo in RPC design doc
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] KUDU-1307 [master] support tables with range partition bounds

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1307 [master] support tables with range partition bounds
..


Patch Set 10:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/2806/10//COMMIT_MSG
Commit Message:

Line 11: not covered by a tablet. A new feature master feature flag has been 
added so
"A new feature master feature flag" is a little awkward, is there an extra 
'feature' here?


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

Line 241: Arena arena(1024, 1024 * 1024);
What's this doing here?


Line 250:   vector>* range_partitions) const {
To Todd's suggestions, perhaps we should DCHECK(range_partitions->empty()), the 
same way you've done that for the other Encode functions?


Line 264:   "Range bound has lower bound equal to or above the upper 
bound",
So FWIW, I was wrong in telling you to use capital letters for the beginning of 
error messages. Todd reminded me of that later; since we append status messages 
to one another, it's best for them to start in lower-case so the composed 
sentences look less awkward. Sorry.


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.h
File src/kudu/common/partition.h:

Line 257: 
Nit: unnecessary new line?


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

Line 43: namespace kudu {
Is this typical? Don't we prefer to put all the "using" statements in one giant 
block together, prefixing with kudu:: if necessary?


Line 95:const 
vector
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1267: Calling getString() on RowResult for wrong type gave weird exception

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1267: Calling getString() on RowResult for wrong type gave 
weird exception
..


Patch Set 1:

Let's assume that your Kudu repo is checked out to whatever local branch has 
this commit:

  1. 
  2. git commit -a --amend
  3. git push gerrit HEAD:refs/for/master

That's really all you need. The key is step #2, which takes the changes you 
made to your working directory in step #1 and merges them into the _existing_ 
commit. In doing so, the Change-Id in the existing commit won't change, and 
when you push to gerrit (step #3) it will recognize the continuity and will 
update your existing change instead of creating a new one.

If you're still having trouble, let me walk you through it on Slack.

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

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

[kudu-CR] KUDU-1386 NaN float and double values are not handled correctly

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-1386 NaN float and double values are not handled correctly
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3142/2/src/kudu/common/types.h
File src/kudu/common/types.h:

Line 303: } else if (isnan(*lhs_n)) {
> given that nans are less common than anything else, I think it would be bes
Done.


Line 334: const double *lhs_n = reinterpret_cast(lhs);
> can you extract this code into a FloatCompare templated method?
Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I194dcddeb8eabcc67699661b9cc9362a99f2f4ae
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Move the PbTracer in rpc_context.cc to pb_util

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Move the PbTracer in rpc_context.cc to pb_util
..


Patch Set 1:

(1 comment)

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

Line 431: class PbTracer : public debug::ConvertableToTraceFormat {
Since you're making this more "public" than it was before (i.e. not buried in 
rpc_context.cc), could you add comments to the methods explaining what they do, 
and perhaps a class comment explaining how to use this thing?


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

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

[kudu-CR] Add unique id generation to the client

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add unique id generation to the client
..


Patch Set 11:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3077/11/src/kudu/client/client_builder-internal.cc
File src/kudu/client/client_builder-internal.cc:

Line 26: default_rpc_timeout_(MonoDelta::FromSeconds(10)) {}
> Can you revert this change? It's just unnecessary noise.
Done


http://gerrit.cloudera.org:8080/#/c/3077/11/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:

Line 42: namespace client {
> Hmm, while the test is called client-stress-test, it's part of the integrat
because I needed to make this test a friend class of client. I tried to make 
the friend class in client.h refer to this test with the kudu:: prefix, but to 
no avail. This is the only way I got it to work without exposing the client_id_ 
with an accessor.
Maybe I missed something. Ideas?


Line 279:  << total_num_rejections << " memory rejections";
> The old indentation was nicer, I think.
Done


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

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

[kudu-CR] service_pool: only log queue overflows once per second

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: service_pool: only log queue overflows once per second
..


service_pool: only log queue overflows once per second

These queue overflow messages are extremely frequent on stress
workloads, and I found that the log spew itself caused the server to
block on glog locks, compounding the problem. Spewing these messages is
rarely helpful, so this patch throttles it to once a second.

Change-Id: I636ef39798f9b2ab7d8aec237285c949ec119468
Reviewed-on: http://gerrit.cloudera.org:8080/3185
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans
---
M src/kudu/rpc/service_pool.cc
1 file changed, 2 insertions(+), 1 deletion(-)

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



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

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

[kudu-CR](branch-0.9.x) service_pool: only log queue overflows once per second

[Todd Lipcon (Code Review)]

Todd Lipcon has uploaded a new change for review.

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

Change subject: service_pool: only log queue overflows once per second
..

service_pool: only log queue overflows once per second

These queue overflow messages are extremely frequent on stress
workloads, and I found that the log spew itself caused the server to
block on glog locks, compounding the problem. Spewing these messages is
rarely helpful, so this patch throttles it to once a second.

Change-Id: I636ef39798f9b2ab7d8aec237285c949ec119468
Reviewed-on: http://gerrit.cloudera.org:8080/3185
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans
(cherry picked from commit 298806fa32c44e986c2401fa5ac416bcbc491523)
---
M src/kudu/rpc/service_pool.cc
1 file changed, 2 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I636ef39798f9b2ab7d8aec237285c949ec119468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-0.9.x
Gerrit-Owner: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will addressing the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear waht it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 341 insertions(+), 0 deletions(-)


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

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

[kudu-CR](branch-0.9.x) rpcz: break out trace metrics into separate PB fields

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: rpcz: break out trace metrics into separate PB fields
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8bd1a249b2f09f8eb63bc64de55147b3d7738a9c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-0.9.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Dependency on Hadoop test classes causes impertinent Javadoc warnings

[Will Berkeley (Code Review)]

Will Berkeley has uploaded a new change for review.

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

Change subject: Dependency on Hadoop test classes causes impertinent Javadoc 
warnings
..

Dependency on Hadoop test classes causes impertinent Javadoc warnings

Initial patch to see what Jenkins says since the tests don't run well
locally for me...will update and clean up again with another patch set
later.

Change-Id: I297f46d930f274666db9d503cf8897fe509470da
---
M java/kudu-mapreduce/pom.xml
A java/kudu-mapreduce/src/main/java/org/kududb/mapreduce/JarFinder.java
M 
java/kudu-mapreduce/src/main/java/org/kududb/mapreduce/KuduTableMapReduceUtil.java
3 files changed, 185 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I297f46d930f274666db9d503cf8897fe509470da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 

[kudu-CR] Remove default table partitioning

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Remove default table partitioning
..


Patch Set 9:

Looks like only the Python tests are still failing now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7021d7950f8dbb4918503ea6fab2e6ee35076064
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] java: use truncated randomized exponential backoff for retries

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: java: use truncated randomized exponential backoff for retries
..


java: use truncated randomized exponential backoff for retries

This changes the Java client to use a randomized exponential backoff,
starting at 1ms, and going up to 4096ms, rather than the previous linear
backoff of (500ms * retry_count). The old backoff was far too
aggressive, and resulted in the Java client regularly having high
percentile latencies of 500 or 1000ms even when there was spare capacity
in the system.

I tested this on a 70 node cluster using a read-only YCSB workload, and
the 99.99th percentile latency dropped from >1s to 390ms.  Combined with
further in-flight changes, the 99.99th percentile dropped further to
~56ms.

Change-Id: I79256e00cf35c072c60cd2b9034e6c1b10c18b38
Reviewed-on: http://gerrit.cloudera.org:8080/3184
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
1 file changed, 3 insertions(+), 2 deletions(-)

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



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

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

[kudu-CR] KUDU-1267: Calling getString() on RowResult for wrong type gave weird exception

[Ted Malaska (Code Review)]

Ted Malaska has abandoned this change.

Change subject: KUDU-1267: Calling getString() on RowResult for wrong type gave 
weird exception
..


Abandoned

Killing this one

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I6147391fb00e58c47ff07ae0d5f0cb5e3a708726
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ted Malaska 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins