Adar Dembo has posted comments on this change. Change subject: WIP: Add new ConnectToMaster RPC, implement client fallback ......................................................................
Patch Set 1: (8 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? Line 84: ConnectToMasterResponsePB* out_; Can we note that out_ isn't owned by this class (I think it's owned by the ConnectToMasterRpc() constructor caller, right)? 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? Line 114: out_->Clear(); Why do we need to explicitly clear it? Doesn't ConnectToMasterAsync() overwrite it fully? PS1, Line 126: this, : Status::OK())); Nit: indentation. Line 139: // TODO(todd): this is the most confusing code I've ever seen... I look forward to your continued contributions on the subject. Line 150: rpc->error_response()->unsupported_feature_flags_size() > 0) { Can we be more specific, though? Can we pinpoint that it was CONNECT_TO_MASTER that was missing, vs. some other feature flag we might add in the future? 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. -- 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-HasComments: Yes
