Todd Lipcon has posted comments on this change. Change subject: WIP: Add new ConnectToMaster RPC, implement client fallback ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/5869/1/src/kudu/client/master_rpc.cc File src/kudu/client/master_rpc.cc: PS1, Line 81: StatusCallback user_cb_; : Sockaddr addr_; > Can we mark these as const? Done Line 84: ConnectToMasterResponsePB* out_; > Can we note that out_ isn't owned by this class (I think it's owned by the Done PS1, Line 111: // TODO(todd): should this be setting an RPC call deadline based on 'deadline'? : // it doesn't seem to be. > Yeah, that's odd. Does everything break if 'deadline' is used? didn't try it, but wanted to keep this as a simple refactor and address that separately Line 114: out_->Clear(); > Why do we need to explicitly clear it? Doesn't ConnectToMasterAsync() overw Done PS1, Line 126: this, : Status::OK())); > Nit: indentation. Done Line 150: rpc->error_response()->unsupported_feature_flags_size() > 0) { > Can we be more specific, though? Can we pinpoint that it was CONNECT_TO_MAS i'm not sure the extra several lines of code here are worth it. These feature flags are call-specific, so any new feature flag we added would be accompanied by a very specific bit of code in this same class (not like some unrelated feature like RPC compression would result in the same error) http://gerrit.cloudera.org:8080/#/c/5869/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 3755: RespClass* resp, RpcContext* rpc) > warning: macro argument should be enclosed in parentheses [misc-macro-paren I don't think this is relevant here http://gerrit.cloudera.org:8080/#/c/5869/1/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: Line 352: void MasterServiceImpl::ConnectToMaster(const ConnectToMasterRequestPB* req, > warning: parameter 'req' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/5869/1/src/kudu/master/master_service.h File src/kudu/master/master_service.h: PS1, Line 88: ConnectToMasterResponsePB* resp, : rpc::RpcContext* rpc) OVERRIDE; > Nit: indentation. Done -- To view, visit http://gerrit.cloudera.org:8080/5869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I879801400ede4209679e6eb14eceb43775916c78 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
