Dan Burkert 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: (11 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 Done http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/hms/hms_catalog.h@37 PS2, Line 37: class HostPort; > nit: alphabetical order Done http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/hms/hms_catalog.h@177 PS2, Line 177: static bool IsFatalError(const Status& status); > Remove? Done http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/hms/hms_catalog.cc@276 PS2, Line 276: Slice hms_database; : Slice hms_table; : RETURN_NOT_OK(ParseTableName(name, &hms_database, &hms_table)); > Could be lifted out of the Execute() call. Done 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@119 PS2, Line 119: // Client options > nit: Move to L121. addresses_ is an option for configuring the underlying thrift client, even if it isn't part of the ClientOptions struct. 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: That would require the user to cast the Client* to the correct concrete type, effectively losing type safety. http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@137 PS2, Line 137: which live in modules which are not : // linked to be the thrift module. > Not quite follow this. Do you mean 'which live in modules to which the thri oops, I had an extraneous word in there. Hopefully it's more clear now, otherwise I can rewrite. http://gerrit.cloudera.org:8080/#/c/11570/2/src/kudu/thrift/client.h@186 PS2, Line 186: // TODO(todd): wrapping this in a TRACE_EVENT scope and a LOG_IF_SLOW and such : // would be helpful. Perhaps a TRACE message and/or a TRACE_COUNTER_INCREMENT : // too to keep track of how much time is spent in calls to HMS for a given : // CreateTable call. That will also require propagating the current Trace : // object into the 'Rpc' object. Note that the HmsClient class already has : // LOG_IF_SLOW calls internally. > Should be updated to remove HMS references. Done 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()`? Done 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? Done 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 Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 05 Oct 2018 18:30:33 +0000 Gerrit-HasComments: Yes
