[kudu-CR] Memory tracking for result tracker
David Ribeiro Alves has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: Verified+1 -- 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: 19 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 has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: theres a bunch of spurious failures, mostly on lint builds. trying once more before overriding -- 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: 19 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
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2541/ -- 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: 19 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
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2540/ -- 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: 19 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
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2539/ -- 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: 19 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
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: Build Started http://104.196.14.100/job/kudu-gerrit/2538/ -- 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: 19 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
Adar Dembo has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 18: (1 comment) I don't have any more comments except on the tests. http://gerrit.cloudera.org:8080/#/c/3627/18/src/kudu/rpc/rpc-stress-test.cc File src/kudu/rpc/rpc-stress-test.cc: Line 260: SleepFor(MonoDelta::FromMilliseconds(100)); I'm concerned that the sleeps will make these tests flaky. Could you loop them a thousand times in TSAN mode? -- 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: Yes
[kudu-CR] Memory tracking for result tracker
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] Memory tracking for result tracker
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] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 18: Build Started http://104.196.14.100/job/kudu-gerrit/2516/ -- 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] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 17: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2514/ -- 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 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 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
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 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
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 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] Memory tracking for result tracker
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 (#15). 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/15 -- 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: 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
[kudu-CR] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 15: Build Started http://104.196.14.100/job/kudu-gerrit/2511/ -- 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: No
[kudu-CR] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 14: Build Started http://104.196.14.100/job/kudu-gerrit/2510/ -- 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: 14 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
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 (#14). 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/14 -- 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: 14 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
David Ribeiro Alves has posted comments on this change.
Change subject: Memory tracking for result tracker
..
Patch Set 13:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS13, Line 78: // Release all the memory for the stuff we'll delete on
destruction.
: for (auto& client_state : clients_) {
: for (auto& completion_record :
client_state.second->completion_records) {
:
mem_tracker_->Release(completion_record.second->memory_footprint());
: }
:
mem_tracker_->Release(client_state.second->memory_footprint());
: }
: mem_tracker_->Release(memory_footprint());
> Can we aggregate this and make just one Release() call? This isn't a hot pa
yeah this isn't a hot path at all, and I found it helpful for debugging to have
separate releases that you can selectively comment out.
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 228: return client_state_map_bytes_;
> Typically memory_footprint() methods are supposed to account for the entire
well the CompletionREcord's memory calculation is not shallow. So it'd be wrong
in that case. I'd rather keep the naming and add some comments.
Line 254: int64_t memory_footprint() const {
> Likewise, memory_footprint_excluding_rpcs.
Seem my comment above
Line 255: return kudu_malloc_usable_size(this)
> Does this method need to be synchronized in some way? Can the CompletionRec
these structs are assumed to be mutated under the lock. I think it makes sense
to have Unlocked/Locked when there's a mix of locking in methods but this is
not the case
Line 281: int64_t memory_footprint() const {
> Likewise, memory_footprint_excluding_completion_records.
See my comment above.
Line 282: return kudu_malloc_usable_size(this) +
completion_record_map_bytes_;
> Do we need to synchronize access to completion_record_map_bytes_?
same as above
PS13, Line 327: // Lock that protects access to 'clients_' and to the state
contained in each
: // ClientState.
: // TODO consider a per-ClientState lock if we find this too
coarse grained.
> Does this also protect access to client_state_map_bytes_? If so, should it
memory_footprint() is only called under the lock.
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:
Line 259: usleep(100 * 1000);
> Nit: Can we use SleepFor() instead? It's more idiomatic for Kudu, and makes
Done. We sleep here because the client gets the answer before the memtracker is
updated
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
Line 102: result_tracker_(new
rpc::ResultTracker(CreateMemTrackerForForResultTracker(mem_tracker_))),
> How about:
actually removed the method above and called CreateTracker inline.
--
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: 13
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
Adar Dembo has posted comments on this change.
Change subject: Memory tracking for result tracker
..
Patch Set 13:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
PS13, Line 78: // Release all the memory for the stuff we'll delete on
destruction.
: for (auto& client_state : clients_) {
: for (auto& completion_record :
client_state.second->completion_records) {
:
mem_tracker_->Release(completion_record.second->memory_footprint());
: }
:
mem_tracker_->Release(client_state.second->memory_footprint());
: }
: mem_tracker_->Release(memory_footprint());
Can we aggregate this and make just one Release() call? This isn't a hot path,
but it seems easy enough to do.
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:
Line 228: return client_state_map_bytes_;
Typically memory_footprint() methods are supposed to account for the entirety
of the object, including any nested allocations. Since you're just using it for
a "shallow" accounting, perhaps call it memory_footprint_excluding_clients, and
add a comment explaining that?
Ahh, the issue is that the ScopedMemTrackerUpdater expects a method named
memory_footprint(). How about renaming all of them to
memory_footprint_shallow() instead, and making it clear via comments which
"deep" structures they are not accounting for?
Line 254: int64_t memory_footprint() const {
Likewise, memory_footprint_excluding_rpcs.
Line 255: return kudu_malloc_usable_size(this)
Does this method need to be synchronized in some way? Can the CompletionRecord
be modified while we're executing here? Or is every access expected to hold
lock_? If so, maybe add Unlocked() to the name of the method.
Line 281: int64_t memory_footprint() const {
Likewise, memory_footprint_excluding_completion_records.
Line 282: return kudu_malloc_usable_size(this) +
completion_record_map_bytes_;
Do we need to synchronize access to completion_record_map_bytes_?
PS13, Line 327: // Lock that protects access to 'clients_' and to the state
contained in each
: // ClientState.
: // TODO consider a per-ClientState lock if we find this too
coarse grained.
Does this also protect access to client_state_map_bytes_? If so, should it be
taken in memory_footprint()?
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:
Line 259: usleep(100 * 1000);
Nit: Can we use SleepFor() instead? It's more idiomatic for Kudu, and makes the
units a little clearer.
Separately, why do we need to sleep here?
http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
PS9, Line 89: }
:
: } /
> Done re the id.
Hmm, OK, I guess that's true. Admittedly a separate memtracker is more of an
issue when its parent is the tablet memtracker, as then a server can have
hundreds or thousands additional memtrackers, which muddies the memory UI view
significantly.
http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
Line 102: result_tracker_(new
rpc::ResultTracker(CreateMemTrackerForForResultTracker(mem_tracker_))),
How about:
result_tracker_(new rpc::ResultTracker(MemTracker::CreateTracker(-1,
"result-tracker", mem_tracker_))),
Or is that too long?
--
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: 13
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 has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 13: Build Started http://104.196.14.100/job/kudu-gerrit/2508/ -- 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: 13 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
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 (#13). 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, 178 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/13 -- 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: 13 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
David Ribeiro Alves has posted comments on this change.
Change subject: Memory tracking for result tracker
..
Patch Set 9:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 75: mem_tracker_->Consume(sizeof(ClientState));
> If we want to make ClientState memory tracking accurate, we need to account
Done
Line 85: mem_tracker_->Consume(sizeof(CompletionRecord));
> Same issues with CompletionRecord and its nested vector of OngoingRpcs (you
Done
Line 234: mem_tracker_->Consume(response->ByteSize());
> Should use SpaceUsed(), I think.
Done
Line 255: mem_tracker_->Release(sizeof(OnGoingRpcInfo));
> We've found that Release() in a loop is generally less efficient than addin
I've changed how this works. as a side effect the mem tracker now gets updated
only once. Not that it matters in this case since in 99.999% of the cases we'll
only seen 1 of these.
http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
PS9, Line 89: if (id != 0) {
: StrAppend(&id_str, " ", id);
: }
> Don't need this; each of these memtrackers is already "disambiguated" by vi
Done re the id.
don't you need a separate memtracker to be able to follow it's specific memory
consumption? knowing how much memory is being used by the result tracker
specifically seems pretty important to be able to tweak gc times.
--
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: 9
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
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 (#12). 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/12 -- 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: 12 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 has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 12: Build Started http://104.196.14.100/job/kudu-gerrit/2507/ -- 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: 12 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
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 11: Build Started http://104.196.14.100/job/kudu-gerrit/2506/ -- 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: 11 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
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 (#11). 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, 178 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/11 -- 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: 11 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 has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 10: Build Started http://104.196.14.100/job/kudu-gerrit/2505/ -- 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: 10 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
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 (#10). 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/10 -- 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: 10 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
Adar Dembo has posted comments on this change.
Change subject: Memory tracking for result tracker
..
Patch Set 9:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:
Line 75: mem_tracker_->Consume(sizeof(ClientState));
If we want to make ClientState memory tracking accurate, we need to account for
two additional things:
1. Malloc "slop". sizeof(ClientState) may not account for the actual size of
the memory allocation used in "new ClientState()". For example, if the size of
the struct is 40 bytes, the allocation may be the next power of 2 (i.e. 64
bytes). We deal with this elsewhere by using kudu_malloc_usable_size(ptr) as
the "size of object"; sometimes this is internalized inside a
memory_footprint() method.
2. The size of any nested heap pointers. ClientState contains a map, and that
map contains nested pointers whose memory consumption isn't being accounted
for. Look at how Schema::name_to_index_ is managed to see how you might do that.
Line 85: mem_tracker_->Consume(sizeof(CompletionRecord));
Same issues with CompletionRecord and its nested vector of OngoingRpcs (you can
use kudu_malloc_usable_size(vector.data()), though you need to check that
vector.capacity() >0 first).
I think you're already taking care of the nested protobuf elsewhere.
Line 234: mem_tracker_->Consume(response->ByteSize());
Should use SpaceUsed(), I think.
Line 255: mem_tracker_->Release(sizeof(OnGoingRpcInfo));
We've found that Release() in a loop is generally less efficient than adding up
all the memory consumed in the loop and making one call to Release() at the
end. That change reduced block manager CPU consumption pretty dramatically on
startup.
http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
PS9, Line 89: if (id != 0) {
: StrAppend(&id_str, " ", id);
: }
Don't need this; each of these memtrackers is already "disambiguated" by virtue
of having a unique parent.
That said, why bother creating a memtracker for the ResultTracker instead of
just reusing the server memtracker? IIRC we only create separate memtrackers
for objects that come and go (e.g. tablet, memrowset, deltamemstore).
--
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: 9
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 has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 9: Build Started http://104.196.14.100/job/kudu-gerrit/2498/ -- 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: 9 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-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Memory tracking for result tracker
David Ribeiro Alves has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 8: Verified+1 unrelated failure (mm test) -- 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: 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-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Memory tracking for result tracker
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 (#8). 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, 106 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/8 -- 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: 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-Reviewer: Todd Lipcon
[kudu-CR] Memory tracking for result tracker
David Ribeiro Alves has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/3627/6//COMMIT_MSG Commit Message: PS6, Line 10: client's > nit: clients' Done -- 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: 6 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-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 8: Build Started http://104.196.14.100/job/kudu-gerrit/2487/ -- 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: 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-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 7: Build Started http://104.196.14.100/job/kudu-gerrit/2485/ -- 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: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: 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
Jean-Daniel Cryans has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/3627/6//COMMIT_MSG Commit Message: PS6, Line 10: client's nit: clients' (regular noun, plural, to show their possession) -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: 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
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 (#6). 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 client's 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, 106 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/3627/6 -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Memory tracking for result tracker
Kudu Jenkins has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/2442/ -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
