David Ribeiro Alves 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? Done 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 Done 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 Done Line 38: const std::shared_ptr<Messenger>& messenger) > I think this header is missing a number of includes like messenger.h, monot Done Line 43: initiliza > typo Done Line 43: // Performs target lookup/initilization. > add a blank line between the above method and this one Done Line 44: // When the target is ready Try() will be called. > I think this could be expanded a bit to make it clearer Done Line 73: // if that is the case. > what's the return value indicate? Done Line 74: virtual bool RetryIfNeeded(const RetriableRpcStatus& result, Server* server) { > hm, does this need to be virtual? same with the other private methods below Done Line 148: rpcs > typo Done -- 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
