Adar Dembo has posted comments on this change. Change subject: Add a generic retriable rpc class ......................................................................
Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/3064/9/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 48: #include "kudu/rpc/retriable_rpc.h" Nit: should come before rpc.h. Line 312: delete this; Nit: could you put 'this' in a local unique_ptr or equivalent at the top of the function instead? I think it's less error-prone (i.e. if the control flow changes, we won't leak the RPC). http://gerrit.cloudera.org:8080/#/c/3064/9/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: Line 31: // A base class for replicated RPCs that handles replica picking and retry logic. Could you also define "replicated RPCs" here? Line 42: class RetriableRpc : public Rpc { FWIW, I think any RPC sent by the client is retryable, even if its target isn't necessarily a "replicated" entity (e.g. ListTabletServers() targets the master process, not the master tablet itself). To that end, I'd also like you to merge Rpc and RetriableRpc into a single Rpc class. In my view there's no difference, and it'd simplify the class hierarchy significantly. Indeed, a complicated three-level hierarchy like this is already stretching the limits of what a sane developer can comprehend; reducing it to two levels would be a huge help. Line 58: // Subclasses implement this method to actually try the RPC. Nit: some parts of this class' documentation uses "RPC" while other parts uses "rpc". Could you pick one and use it consistently? Line 62: // Subclasses implement this method to analyze 'status', the controller status or : // the response and return a RetriableRpcStatus which will then be used : // to decide how to proceed (retry or give up). At least some of this logic should be shared across RPC types. Not sure whether it's best to do that with a base class implementation or some sort of helper method. Line 71: // Request body. : RequestPB req_; : : // Response body. : ResponsePB resp_; How do you feel about making these private and forcing subclasses to access them via mutator methods? I'm not a huge fan of protected fields, but I don't feel strongly and I'd like to hear what you (or others) think. Line 101: void RetriableRpc<Server,RequestPB,ResponsePB>::SendRpc() { Nit: void RetriableRpc<Server, RequestPB, ResponsePB> Below too. Line 151: Finish(status); Shouldn't this pass result.status? http://gerrit.cloudera.org:8080/#/c/3064/9/src/kudu/rpc/rpc.h File src/kudu/rpc/rpc.h: Line 25: #include "kudu/rpc/response_callback.h" What's this for? -- 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: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
