Todd Lipcon has posted comments on this change. ( )

Change subject: KUDU-2297 (part 5): dump stacks into diagnostics log on service 
queue overflow

Patch Set 5:

File src/kudu/master/
PS5, Line 829: Env::Default()
> KuduTest has an env_ member you can use. Below too.
PS5, Line 835:       return line.find("service queue overflowed for 
kudu.master.MasterService") != string::npos;
> Would MatchPattern be more idiomatic? Won't have to worry about string::npo
File src/kudu/rpc/service_pool.h:
PS5, Line 67:   void set_reject_too_busy_hook(std::function<void(void)> hook) {
> Nit: if this is supposed to look like a setter for too_busy_hook_, I think
File src/kudu/rpc/
PS5, Line 136:   if (too_busy_hook_) {
             :     too_busy_hook_();
             :   }
> You may get a less noisy stack if you call this before responding to the RP
I think though it's best to respond as quickly as possible because it frees up 
the memory used by the inbound request
File src/kudu/server/
PS5, Line 196:         WARN_NOT_OK(s, "Unable to collect stacks to diagnostics 
> Nit: "Will try again in ..." ?
The next patch in this series gets rid of this anyway, so I'll skip this one if 
you don't mind.
PS5, Line 201:       dump_stacks_now_reason_ = boost::none;
> Why do this here and not on L187, right after storing a local copy in 'reas
I actually think that's somewhat advantageous because if you get "requested" to 
dump stacks while you're already dumping stacks, there isn't really any need to 
wake up and dump stacks again (presumably your previous dump was already more 
accurate). Again if you disagree would rather change it in the follow-up patch 
which restructures this code path. In the end though it doesn't really matter 
much since it's not a super likely race and the only harm is an extra (or 
missing) stack dump which in most cases no one will ever even look at.
File src/kudu/server/rpc_server.h:
PS5, Line 62:   void 
set_reject_too_busy_hook(std::function<void(rpc::ServicePool*)> hook) {
> Nit: same comment about setter naming.

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8
Gerrit-Change-Number: 9375
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Thu, 08 Mar 2018 02:55:43 +0000
Gerrit-HasComments: Yes

Reply via email to