Adar Dembo 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: (6 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. 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::npos comparisons then. 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 set_too_busy_hook() would be a better name. 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 ..." ? 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 'reason'? By doing it down here it's possible for another thread to overwrite dump_stacks_now_reason_ during the stack logging. 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. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 07 Mar 2018 19:27:44 +0000 Gerrit-HasComments: Yes
