[kudu-CR] Add garbage collection to ResultTracker

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add garbage collection to ResultTracker

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 18: -Verified

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: Add garbage collection to ResultTracker

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
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-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 183 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/18
-- 
To view, visit http://gerrit.cloudera.org:8080/3627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Add garbage collection to ResultTracker

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 15:

(7 comments)

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

Line 47: struct ScopedMemTrackerUpdater {
> would using ScopedCleanup with a lambda be easier? eg:
interesting. you're right I didn't notice that before. noted, thanks.


Line 70: ResultTracker::ResultTracker(const shared_ptr& mem_tracker)
> I think with C++11 it's preferable to take shared_ptr mem_track
Done


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

PS15, Line 228: ns'
> typo
Done


Line 231: return client_state_map_bytes_;
> this needs an ANNOTATE_UNPROTECTED_READ or a lock, right?
this is only read under the lock (as are map changes), we don't need the 
annotation right?


http://gerrit.cloudera.org:8080/#/c/3627/15/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:

PS15, Line 259: Sleep t
> is this fragile? why not a loop? also, why is there a delay here? shouldn't
the memtracker gets updated by the scoped stuff, so its likely that the client 
gets the response before it is updated. I'd loop but not sure what to make a 
loop condition out of. Specifically there might be multiple updates, so we're 
not sure when they are all over and we can't know the size specifically.


Line 262: ASSERT_GT(mem_consumption, original_resp.SpaceUsed());
> I think grabbing the original consumption before the RPC, and making sure t
Done


PS15, Line 283: double 
> hm, I'm actually slightly surprised by this. Isn't this pretty dependent on
This was left over from the original impl that used sizeof and didn't measure 
the maps, apparently its still true.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 17:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
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-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 183 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/17
-- 
To view, visit http://gerrit.cloudera.org:8080/3627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 16:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
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-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 182 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/16
-- 
To view, visit http://gerrit.cloudera.org:8080/3627
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 15:

(7 comments)

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

Line 47: struct ScopedMemTrackerUpdater {
would using ScopedCleanup with a lambda be easier? eg:

auto update_memtracker = MakeScopedCleanup([]{
  tracker->Release(...)
});

on second thought, now that I understand it, maybe not... but leaving the 
comment just because MakeScopedCleanup is neat and you might not have seen it 
when it was added :)


Line 70: ResultTracker::ResultTracker(const shared_ptr& mem_tracker)
I think with C++11 it's preferable to take shared_ptr mem_tracker 
(byvalue) and then assign with std::move


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

PS15, Line 228: ns'
typo


Line 231: return client_state_map_bytes_;
this needs an ANNOTATE_UNPROTECTED_READ or a lock, right?


http://gerrit.cloudera.org:8080/#/c/3627/15/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:

PS15, Line 259: Sleep t
is this fragile? why not a loop? also, why is there a delay here? shouldn't it 
be updated already before AddExactlyOnce returns?


Line 262: ASSERT_GT(mem_consumption, original_resp.SpaceUsed());
I think grabbing the original consumption before the RPC, and making sure that 
it _increases_ by more than SpacedUsed would be more robust, since the 
"SpaceUsed" of this protobuf is pretty small, potentially even less than the 
base size of the structure


PS15, Line 283: double 
hm, I'm actually slightly surprised by this. Isn't this pretty dependent on the 
implementation of the map?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

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


Patch Set 5:

We should make sure not to forget about this one before calling all the replay 
cache work "done"

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

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

[kudu-CR] rw mutex: prevent recursive use

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: rw_mutex: prevent recursive use
..


Patch Set 6: Code-Review+2

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

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