David Ribeiro Alves has posted comments on this change. Change subject: Refactor retry handling logic for writes ......................................................................
Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/2970/7/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 48: #include "kudu/rpc/replicated_rpc.h" > Nit: before rpc.h. Done Line 213: can be used to choose the action to take > Nit: the enum doesn't enable a "choice" per se; it explicitly tells the cal well it doesn't tell the caller to empty the cache or retry, just puts the result into a few buckets, the caller then decides what to do. for instance the implementation changing on REPLICA_TOO_BUSY to try another replica instead of the same one Line 403: void WriteRpc::LookupTabletCb(const Status& status) { > Shouldn't this also call into Analyze/Retry? In theory it should; in practi this disappears in the next patch. Line 444: if (result.status.IsRemoteError()) { > I don't think this affects control flow but should improve readability: cou Done Line 445: const ErrorStatusPB* err = mutable_retrier()->controller().error_response(); > So part of this refactor (maybe not in this patch) will include the removal maybe, I'm creating a ReplicatedRpc that will encapsulate some logic. Not sure the retrier should be there (less impl inheritance). Line 508: DCHECK(found) << "Tablet " << tablet_->tablet_id() << ": Unable to mark replica " > Nit: can you rewrite this line using strings::Substitute() so it's more cle Done Line 528: default: { > Nit: why the new scope? Done Line 537: > Two things: 1: honestly don't think it improves much, and adds an extra copy, but ok. 2: Done http://gerrit.cloudera.org:8080/#/c/2970/7/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 78: s = Status::NetworkError("No addresses for " + hp.ToString()); > I know you asked me about this in chat, but could you also mention it in th todd asked for a test for this so moving this to its own patch. -- To view, visit http://gerrit.cloudera.org:8080/2970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0e0d491f902191c88c58e3d627106cc1be1bb3cc Gerrit-PatchSet: 7 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
