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 <t...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 07 Mar 2018 19:27:44 +0000
Gerrit-HasComments: Yes

Reply via email to