Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8789 )
Change subject: KUDU-2228: Make Messenger options configurable ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@168 PS3, Line 168: int64_t rpc_negotiation_timeout_ms_ = 3000; Probably best to set these defaults in the constructor to keep things consistent. http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@74 PS3, Line 74: // Default of 65 seconds. nit: no need to comment this (it's obvious from context). http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/negotiation.h@33 PS3, Line 33: enum class TriStateFlag; This doesn't appear to be used. Edit: or is it necessary because RpcAuthentication and RpcEncryption are typedefs? http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@564 PS3, Line 564: void StartTestServer(Sockaddr *server_addr, bool enable_ssl = false, Wrap this like you did with 'CreateMessenger' http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/rpc-test-base.h@605 PS3, Line 605: void DoStartTestServer(Sockaddr *server_addr, bool enable_ssl = false, ditto http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/sasl_common.cc@269 PS3, Line 269: is_kerberos_enabled.Store(kerberos_enabled); I think it may be cleaner to pass in the kerberos_enabled flag into the Once, and set it there. That will make it threadsafe as just a normal (non-atomic) static. Unfortunately it doesn't look like the Google flavor of once supports flags to the init function, but the standard library does, so you'd have to switch it to use that. http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/server/server_base.cc@52 PS3, Line 52: // for RpcAuthentication, RpcEncryption this comment isn't necessary, we have a tool that checks headers. -- To view, visit http://gerrit.cloudera.org:8080/8789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba Gerrit-Change-Number: 8789 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 07 Dec 2017 22:02:56 +0000 Gerrit-HasComments: Yes
