David Ribeiro Alves has posted comments on this change.

Change subject: Memory tracking for result tracker

Patch Set 13:


File src/kudu/rpc/result_tracker.cc:

PS13, Line 78:   // Release all the memory for the stuff we'll delete on 
             :   for (auto& client_state : clients_) {
             :     for (auto& completion_record : 
client_state.second->completion_records) {
             :     }
             :   }
             :   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.

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) + 
> 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.

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 

File src/kudu/server/server_base.cc:

Line 102:       result_tracker_(new 
> 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 <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