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<MemTracker>& mem_tracker)
I think with C++11 it's preferable to take shared_ptr<MemTracker> 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 <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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to