Todd Lipcon has posted comments on this change.

Change subject: Memory tracking for result tracker
......................................................................


Patch Set 20:

(3 comments)

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

Line 109:       request_id.seq_no(),
> Your change to push a MemTrackerAllocator into ClientState means the per-cl
isn't that handled by the 
'mem_Tracker_->Consume(client_state->memory_footprint())' above on line 102?


Line 296: 
> See above.
the ClientState's memory usage is already released through an explicit 
->Release(client_state->memory_footprint()) (only in the destructor in this 
patch)

Note that memtrackers in debug modes will verify that on destruction, their 
usage is '0', so if you're convinced that the memory is properly 'Consumed', 
there's a built-in assertion that we have the proper 'Release'


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

PS20, Line 57:     auto release = memory_before - tracked->memory_footprint();
             :     tracker->Release(release);
> Seems like an odd stylistic change, but OK.
oops, was due to some logging i'd added for debug and removed.


-- 
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: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to