You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/07/13 01:18:53 UTC
[kudu-CR] WIP: Add garbage collection to ResultTracker
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3628
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
5 files changed, 155 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/1
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 12: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/2527/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/3628/15//COMMIT_MSG
Commit Message:
Line 9: This adds time and watermark based garbage collection to the ResultTracker.
> Nit: the line is too long.
Done
Line 26: multithreaded threaded test that runs GC at the same time as writes.
> Nit: an extra 'threaded'
Done
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 502: FLAGS_remember_responses_ttl_ms = 100;
> Is it OK that the modified flags are not returned back to their default val
we already wrap all of our tests in google::FlagSaver since the KuduTest base class has a FlagSaver member. I think some older tests that predate that addition have their own FlagSavers - could probably remove them now.
Line 540: // This test creates a thread continuously makes requests to the server, some lasting longer
> Nit: 'continuously makes' ---> 'continuously making'
Done
Line 548: FLAGS_remember_responses_ttl_ms = 10;
> Ditto: consider adding google::FlagSaver to restore default values for the
same
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
Line 126: required int64 first_incomplete_seq_no = 3;
> Any concerns on backward compatibility here? If yes, does it make sense to
RequestIDPB was only added during this release cycle, so no issue with older versions. The whole RequestIDPB itself is optional in the RPC header
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
Line 104: RpcContext* ctx = new RpcContext(call,
> Wouldn't it be a memory leak if the state was one of COMPLETED, IN_PROGRESS
ah, good catch... I wonder why leak sanitizer builds are not catching this. Will investigate.
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the
ResultTracker. Regarding time GC, there are two ttl's, a client ttl and
a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we GC the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we GC
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Reviewed-on: http://gerrit.cloudera.org:8080/3628
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 797 insertions(+), 428 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 17:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/CMakeLists.txt
File src/kudu/rpc/CMakeLists.txt:
Line 121: ADD_KUDU_TEST(exactly_once_rpc-test)
> Can you resort the tests?
Done
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS17, Line 152: completion_record->state = RpcState::IN_PROGRESS;
: completion_record->driver_attempt_no = request_id.attempt_no();
> Can we add these to the CR constructor?
Done
Line 232: completion_record->last_updated = MonoTime::Now();
> Technically we haven't mutated the CR, so why update its timestamp?
yea, not sure, will remove.
Line 348: completion_record->last_updated = MonoTime::Now();
> Likewise, shouldn't we push this down into where we actually update the CR?
not convinced of this one -- if we moved it inside the loop, it might get updated multiple times, and no sense in wasting the extra clock calls.
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
PS17, Line 1247: // We only replay committed requests so the result of tracking this request can be:
: // NEW - This is a previously untracked request, or we changed the driver -> store the result
: // COMPLETED - We've bootstrapped this tablet twice, and previously stored the result -> do
: // nothing.
> Update this comment.
Done
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
PS17, Line 227: VLOG(1) << state()->result_tracker() << " Follower Rpc was not NEW or IN_PROGRESS: "
: << rpc_state << " OpId: " << state()->op_id().ShortDebugString()
: << " RequestId: " << state()->request_id().ShortDebugString();
> Update this too? Should have been "not NEW or COMPLETED", I think.
done. also changed to VLOG(2) since this is quite internal
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS5, Line 45:
> nit: shouldn't whitespaces trail for multi-line strings?
not sure that's a rule. but Done
PS5, Line 91: // If the arriving request is older than our per-client GC watermark, report its
: // staleness to the client.
> This is fatal, right? There's nothing the client can do?
you mean fatal in the sense that the client can't recover? yeah (not in the "crash the server" sense though)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
PS5, Line 227: ClientSttate
> nit
Done
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#10).
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
WIP: Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.
This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 249 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/10
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 9:
Build Started http://104.196.14.100/job/kudu-gerrit/2519/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has uploaded a new patch set (#2).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
5 files changed, 155 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/2
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 18: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 348: int64_t seq_no = request_id.seq_no();
> not convinced of this one -- if we moved it inside the loop, it might get u
Yes, but what of the inverse: if MustHandleRpc==false for all ongoing RPCs, we're changing last_updated for a CR that didn't actually change.
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 17:
Yea, gerrit's poor at dealing with renames. git show -M does a better job. For your convenience, here's the git show -M for the test:
https://gist.github.com/toddlipcon/c150de547f7a6b8462eeaeb568efdae9
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2356/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 7:
Build Started http://104.196.14.100/job/kudu-gerrit/2515/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2352/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 12:
Build Started http://104.196.14.100/job/kudu-gerrit/2525/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 6:
Build Started http://104.196.14.100/job/kudu-gerrit/2497/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 348: completion_record->last_updated = MonoTime::Now();
> Yes, but what of the inverse: if MustHandleRpc==false for all ongoing RPCs,
yea, but the record was still "touched" in a sense, so it's still good to bump its expiration time I think. Maybe "last_touched" would be a better name
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#18).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the
ResultTracker. Regarding time GC, there are two ttl's, a client ttl and
a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we GC the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we GC
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 797 insertions(+), 428 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/18
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 13:
Build Started http://104.196.14.100/job/kudu-gerrit/2537/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#17).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the
ResultTracker. Regarding time GC, there are two ttl's, a client ttl and
a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we GC the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we GC
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 786 insertions(+), 423 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/17
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 16:
Build Started http://104.196.14.100/job/kudu-gerrit/2925/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 17:
Build Started http://104.196.14.100/job/kudu-gerrit/2929/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 10:
Build Started http://104.196.14.100/job/kudu-gerrit/2522/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS5, Line 45:
nit: shouldn't whitespaces trail for multi-line strings?
PS5, Line 91: // If the arriving request is older than our per-client GC watermark, report its
: // staleness to the client.
This is fatal, right? There's nothing the client can do?
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
PS5, Line 227: ClientSttate
nit
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#6).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 163 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/6
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 17:
(6 comments)
It's pretty tough to review exactly_once_rpc-test.cc since gerrit is showing it as all new. Any chance you can break out the rename of the test into a separate patch? Alternatively, you can describe what's actually changed in the test and I can focus on that.
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/CMakeLists.txt
File src/kudu/rpc/CMakeLists.txt:
Line 121: ADD_KUDU_TEST(exactly_once_rpc-test)
Can you resort the tests?
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS17, Line 152: completion_record->state = RpcState::IN_PROGRESS;
: completion_record->driver_attempt_no = request_id.attempt_no();
Can we add these to the CR constructor?
Line 232: completion_record->last_updated = MonoTime::Now();
Technically we haven't mutated the CR, so why update its timestamp?
Line 348: completion_record->last_updated = MonoTime::Now();
Likewise, shouldn't we push this down into where we actually update the CR? I think that's L365, though I'm not sure.
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
PS17, Line 1247: // We only replay committed requests so the result of tracking this request can be:
: // NEW - This is a previously untracked request, or we changed the driver -> store the result
: // COMPLETED - We've bootstrapped this tablet twice, and previously stored the result -> do
: // nothing.
Update this comment.
http://gerrit.cloudera.org:8080/#/c/3628/17/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
PS17, Line 227: VLOG(1) << state()->result_tracker() << " Follower Rpc was not NEW or IN_PROGRESS: "
: << rpc_state << " OpId: " << state()->op_id().ShortDebugString()
: << " RequestId: " << state()->request_id().ShortDebugString();
Update this too? Should have been "not NEW or COMPLETED", I think.
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 18: Verified+1
the test failure was some weird dist-test flakiness that I'm looking into separately. Overriding since this got a bunch of prior jenkins +1s with only surface-level changes since then
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
Line 104: RpcContext* ctx = new RpcContext(call,
> ah, good catch... I wonder why leak sanitizer builds are not catching this.
oh.. it's some slightly weird design going on here -- ResultTracker itself will have responded to the RPC in these cases, and thus deleted the context itself. I added a comment to that effect since it does _look_ like a leak.
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#16).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the
ResultTracker. Regarding time GC, there are two ttl's, a client ttl and
a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we GC the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we GC
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 784 insertions(+), 423 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/16
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 13:
looking over this stuff tonight, just a note: seems like this is lacking actually _triggering_ the GC. I guess we either need some new thread or the ability to do it inline based on memtracker consumption
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15: Code-Review+1
(7 comments)
http://gerrit.cloudera.org:8080/#/c/3628/15//COMMIT_MSG
Commit Message:
Line 9: This adds time and watermark based garbage collection to the ResultTracker.
Nit: the line is too long.
Line 26: multithreaded threaded test that runs GC at the same time as writes.
Nit: an extra 'threaded'
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 502: FLAGS_remember_responses_ttl_ms = 100;
Is it OK that the modified flags are not returned back to their default values? May be, it's safer to add google::FlagSaver here?
Line 540: // This test creates a thread continuously makes requests to the server, some lasting longer
Nit: 'continuously makes' ---> 'continuously making'
Line 548: FLAGS_remember_responses_ttl_ms = 10;
Ditto: consider adding google::FlagSaver to restore default values for the flags.
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
Line 126: required int64 first_incomplete_seq_no = 3;
Any concerns on backward compatibility here? If yes, does it make sense to add corresponding information into the release notes?
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:
Line 104: RpcContext* ctx = new RpcContext(call,
Wouldn't it be a memory leak if the state was one of COMPLETED, IN_PROGRESS or STALE? If yes, then please consider using unique_ptr<> wrapper around the newly allocated RpcContext instance.
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 13:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 64: } // namespace
nit: anonymous namespace, otherwise it looks like namespace name is missing.
Line 94: RetriableRpcStatus status;
nit: is it possible to make status local variable for related scopes only? It does not look like the status variable needs to go though all the scopes to accumulate some changes on its initial value.
Line 243: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Do we want these pseudo-random numbers to repeat in the same sequence for every run of the test? If not, consider initializing random with seed, i.e. calling srandom(time(nullptr)) or something like that before the while() cycle.
Line 257: rand() % (2 * FLAGS_remember_responses_ttl_msecs)));
Ditto for the rand().
Line 259: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Ditto for the rand().
Line 270: // Stubbornly sends the same request to the server, this should observe three states
Nit: period is missing in the end of the sentence.
Line 271: // The request should be successful at first, then its result should be gc'd and the the
typo: strayed extra 'the'.
Line 297: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Ditto for the rand().
Line 513: // This test creates a thread continuously makes requests to the server, some lasting longer
nit: makes --> making
http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 262: // The timestamp of the last time this CompletionRecord was updated.
nit: 'The timestamp of the last CompletionRecord update.' ?
Line 301: SequenceNumber stale_before_seq_no;
Is it OK to leave it uninitialized in the constructor (I didn't see it was initialized).
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
PS15, Line 84: cncurrently
concurrently + period
PS15, Line 86:
word missing
PS15, Line 95: For
Extra word? Something is off in this sentence.
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 40: unique_ptr<RequestIdPB> request_id(new RequestIdPB());
So I know this is a little late to the game, but RpcController holding it's request id by unique_ptr is pretty odd. I can't figure out if it's because it's a heavyweight object, or because it's optional. Looks to me like it should be ~40 bytes + whatever object overhead.
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 225: // Runs garbage collection on the results this result tracker is caching.
I would change this to 'Runs time-based garbage collection...' to distinguish it from the other GC type.
PS15, Line 307: MustGcRecordFunc func
Ideally this would be a template param instead of a std::function so that it can get inlined (I imagine this is perf. sensitive since it's done under a spinlock).
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
Line 126: required int64 first_incomplete_seq_no = 3;
> RequestIDPB was only added during this release cycle, so no issue with olde
All of these fields should be optional, we shouldn't be introducing any more required proto fields.
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
Line 1253: || state == ResultTracker::RpcState::COMPLETED
|| on previous lines
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2469/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS5, Line 91: // If the arriving request is older than our per-client GC watermark, report its
: // staleness to the client.
> coming back as an RPC error (we don't cache results for individual rows, on
ok, makes sense
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15:
Build Started http://104.196.14.100/job/kudu-gerrit/2892/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#15).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we GC the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we GC
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded threaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 782 insertions(+), 423 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/15
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 11:
Build Started http://104.196.14.100/job/kudu-gerrit/2523/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 18:
kicked off dist-test runs with 1000 of each of the integration tests here:
http://dist-test.cloudera.org/job?job_id=todd.1471323037.16714
http://dist-test.cloudera.org/job?job_id=todd.1471323329.17166
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 14:
Build Started http://104.196.14.100/job/kudu-gerrit/2848/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 12: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/2526/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#14).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client to
tell us what's its lowest incomplete sequence number and we gc
everything below that.
This adds a simple test that makes sure this basically works, and adds a
multithreaded threaded test that runs GC at the same time as writes.
NOTE: this does not wire the time-based garbage collection process into
the server itself -- it's currently only triggered by the included
tests.
Original patch by David.
Some changes by Todd.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
D src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
11 files changed, 782 insertions(+), 423 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/14
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#12).
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
WIP: Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.
This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 248 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/12
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS5, Line 91: // If the arriving request is older than our per-client GC watermark, report its
: // staleness to the client.
> Yeah that's what I meant. So is this coming back as a row error or an RPC e
coming back as an RPC error (we don't cache results for individual rows, only for whole batches)
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#11).
Change subject: WIP: Add time/watermark based garbage collection to ResultTracker
......................................................................
WIP: Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.
This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 250 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/11
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#4).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 162 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/4
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 15:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
PS15, Line 84: cncurrently
> concurrently + period
Done
PS15, Line 86:
> word missing
Done
PS15, Line 95: For
> Extra word? Something is off in this sentence.
does a comma after 'batches' make it read better?
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 40: unique_ptr<RequestIdPB> request_id(new RequestIdPB());
> So I know this is a little late to the game, but RpcController holding it's
yea, it's a little strange. want to file a follow-up JIRA? It's baked throughout a bunch of places here.
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 225: // Runs garbage collection on the results this result tracker is caching.
> I would change this to 'Runs time-based garbage collection...' to distingui
Done
PS15, Line 307: MustGcRecordFunc func
> Ideally this would be a template param instead of a std::function so that i
Done
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
Line 126: required int64 first_incomplete_seq_no = 3;
> All of these fields should be optional, we shouldn't be introducing any mor
why? given this struct itself is optional, isn't it safe for its first introduction to have required fields?
In this case though I guess I agree that semantically it's fine for it to be missing (it just means no GC). I'm guessing David made it required so that we dont have cases with misbehaving clients which don't set it and thus end up causing the server to exhaust a lot of memory?
http://gerrit.cloudera.org:8080/#/c/3628/15/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
Line 1253: || state == ResultTracker::RpcState::COMPLETED
> || on previous lines
Done
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add garbage collection to ResultTracker
......................................................................
Patch Set 8:
Build Started http://104.196.14.100/job/kudu-gerrit/2518/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 13:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:
Line 64: } // namespace
> nit: anonymous namespace, otherwise it looks like namespace name is missing
Done
Line 94: RetriableRpcStatus status;
> nit: is it possible to make status local variable for related scopes only?
changed to use brace-initialization for the Status structs at each return site
Line 120: status.result = RetriableRpcStatus::OK;
hrm, I'm curious why David removed this 'still report errors' bit here. will try to dig
Line 243: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
> Do we want these pseudo-random numbers to repeat in the same sequence for e
added SeedRandom() to test setup
Line 257: rand() % (2 * FLAGS_remember_responses_ttl_msecs)));
> Ditto for the rand().
since it's multi-threaded, I don't know that a specific random seed will actually make it use the same sequence, but the above SeedRandom() I added should help for any single-threaded test cases
Line 270: // Stubbornly sends the same request to the server, this should observe three states
> Nit: period is missing in the end of the sentence.
Done
Line 271: // The request should be successful at first, then its result should be gc'd and the the
> typo: strayed extra 'the'.
Done
Line 297: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
> Ditto for the rand().
Done
Line 513: // This test creates a thread continuously makes requests to the server, some lasting longer
> nit: makes --> making
Done
http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 262: // The timestamp of the last time this CompletionRecord was updated.
> nit: 'The timestamp of the last CompletionRecord update.' ?
Done
Line 301: SequenceNumber stale_before_seq_no;
> Is it OK to leave it uninitialized in the constructor (I didn't see it was
Done
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS5, Line 45:
> not sure that's a rule. but Done
Looking almost anywhere else it's done with trailing whitespaces.
PS5, Line 91: // If the arriving request is older than our per-client GC watermark, report its
: // staleness to the client.
> you mean fatal in the sense that the client can't recover? yeah (not in the
Yeah that's what I meant. So is this coming back as a row error or an RPC error?
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Patch Set 18:
Build Started http://104.196.14.100/job/kudu-gerrit/2941/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#5).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 162 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/5
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2467/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#9).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This adds time based garbage collection to the ResultTracker.
There are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 224 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/9
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2351/
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#13).
Change subject: Add time/watermark based garbage collection to ResultTracker
......................................................................
Add time/watermark based garbage collection to ResultTracker
This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.
This adds a simple test that makes sure this basically
works, and adds a multithreaded threaded test that runs GC at the
same time as writes.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/CMakeLists.txt
R src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
10 files changed, 374 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/13
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#7).
Change subject: WIP: Add garbage collection to ResultTracker
......................................................................
WIP: Add garbage collection to ResultTracker
This still needs testing and hooking up to the mm.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 160 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/7
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add garbage collection to ResultTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3628
to look at the new patch set (#8).
Change subject: Add garbage collection to ResultTracker
......................................................................
Add garbage collection to ResultTracker
This adds time based garbage collection to the ResultTracker.
There are two ttl's, a client ttl and a response ttl.
After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.
After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.
This adds a simple test that makes sure this basically
works, but is still missing a proper integration test, which
will come later, when this is integrated with the
MaintenanceManager.
Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
7 files changed, 224 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/3628/8
--
To view, visit http://gerrit.cloudera.org:8080/3628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins