Todd Lipcon has posted comments on this change. Change subject: Add a generic retriable rpc class ......................................................................
Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/3064/7//COMMIT_MSG Commit Message: Line 13: Children by Children do you mean derived classes? http://gerrit.cloudera.org:8080/#/c/3064/7/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 207: virtual void Try(RemoteTabletServer* replica, const ResponseCallback& callback) OVERRIDE; just use c++11 'override' here http://gerrit.cloudera.org:8080/#/c/3064/7/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: Line 33: template <class Server, class RequestPB, class ResponsePB> some docs on the 'Server' template parameter would be nice Line 38: const std::shared_ptr<Messenger>& messenger) I think this header is missing a number of includes like messenger.h, monotime, refcounted, etc Line 43: initiliza typo Line 43: // Performs target lookup/initilization. add a blank line between the above method and this one Line 44: // When the target is ready Try() will be called. I think this could be expanded a bit to make it clearer Line 73: // if that is the case. what's the return value indicate? Line 74: virtual bool RetryIfNeeded(const RetriableRpcStatus& result, Server* server) { hm, does this need to be virtual? same with the other private methods below. Given the length of this method, might be easier to read if you out-of-lined the implementation down below Line 148: rpcs typo -- To view, visit http://gerrit.cloudera.org:8080/3064 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa58bdc5656a5d4d9172885a67363f74718a0c8e Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
