Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11570 )
Change subject: Extract connection retrying and HA support from HmsCatalog ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/11570/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11570/2//COMMIT_MSG@19 PS2, Line 19: No new tests are provided, since the class is tested by the existing IIUC this only tests HaClient<HmsClient>. Maybe until we're ready for more extensive testing, we could at least include a test that just instantiates HaClient<SentryClient>? http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h File src/kudu/thrift/client.h: http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@136 PS2, Line 136: HaClient is defined inline so that it can be instantiated with HmsClient and One way around this is to do something like: class thrift::Client { Status Start(); Status Stop(); bool IsConnected(); string service_name(); }; class HmsClient : thrift::Client {}; class SentryClient : thrift::Client {}; class thrift::HaClient { private: thrift::Client service_client_; }; Then you should be able to put HaClient's definition in its own *.cc, at the cost of using polymorphism. http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@262 PS2, Line 262: VLOG(1) << "Call to " << Service::kServiceName << " failed: " << task_status.ToString(); nit: mind rewriting this with `Substitute()`? http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@274 PS2, Line 274: << "Call to " << Service::kServiceName << " failed after " : << options_.retry_count << " retries: " << first_failure.ToString(); And this? http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@305 PS2, Line 305: VLOG(1) << "Connected to " << Service::kServiceName << " " << address.ToString(); And this -- To view, visit http://gerrit.cloudera.org:8080/11570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8135f4c3995bba0ba28384c35696a1771ff5296 Gerrit-Change-Number: 11570 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 03 Oct 2018 03:48:32 +0000 Gerrit-HasComments: Yes
