Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9375 )
Change subject: KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/master/master-test.cc@829 PS5, Line 829: Env::Default() > KuduTest has an env_ member you can use. Below too. Done http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/master/master-test.cc@835 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 Done http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.h File src/kudu/rpc/service_pool.h: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.h@67 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 Done http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.cc File src/kudu/rpc/service_pool.cc: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/rpc/service_pool.cc@136 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 http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc@196 PS5, Line 196: WARN_NOT_OK(s, "Unable to collect stacks to diagnostics log"); > 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. http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/diagnostics_log.cc@201 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. http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/rpc_server.h File src/kudu/server/rpc_server.h: http://gerrit.cloudera.org:8080/#/c/9375/5/src/kudu/server/rpc_server.h@62 PS5, Line 62: void set_reject_too_busy_hook(std::function<void(rpc::ServicePool*)> hook) { > Nit: same comment about setter naming. Done -- To view, visit http://gerrit.cloudera.org:8080/9375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8 Gerrit-Change-Number: 9375 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 02:55:43 +0000 Gerrit-HasComments: Yes
