David Ribeiro Alves 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. Done Line 312: delete this; > Nit: could you put 'this' in a local unique_ptr or equivalent at the top of Done 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? I meant retriable, Done. Line 42: class RetriableRpc : public Rpc { > FWIW, I think any RPC sent by the client is retryable, even if its target i I sort of agree. Right now this class will handle adding client ids and sequence numbers only to writes. In the future (around milestone 2/3 of the replay cache design) I do think that we could expand this to all classes and then we could merge the hierarchies. Left a TODO for that. Line 58: // Subclasses implement this method to actually try the RPC. > Nit: some parts of this class' documentation uses "RPC" while other parts u Done 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 whe Don't have the bandwidth to do it now, though. Also if you look at the implementation it interacts with member of this class. Line 71: // Request body. : RequestPB req_; : : // Response body. : ResponsePB resp_; > How do you feel about making these private and forcing subclasses to access Seems like more cruft to me. But I don't feel strongly either. Line 101: void RetriableRpc<Server,RequestPB,ResponsePB>::SendRpc() { > Nit: void RetriableRpc<Server, RequestPB, ResponsePB> Done Line 151: Finish(status); > Shouldn't this pass result.status? ah, good catch. Done 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? 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: 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
