Adar Dembo has posted comments on this change. Change subject: Add an all-virtual ReplicaPicker class and a meta cache backed implementation for replicated rpcs ......................................................................
Patch Set 4: (18 comments) The code motion is somewhat tricky so I'm not 100% confident; would be good to get another set of eyes on this. http://gerrit.cloudera.org:8080/#/c/3017/4//COMMIT_MSG Commit Message: Line 13: readies the replica before handing it hover to WriteRpc. This makes the writing Nit: over, not hover Line 14: logic simples (less callbacks) and makes the replica picking code possibly reusable. Nit: simpler, not simples http://gerrit.cloudera.org:8080/#/c/3017/4/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 178: // A ReplicaPicker for tablets backed by the metacache. Nit: MetaCache (since you've said "ReplicaPicker", another class name). Line 180: class MetaCacheReplicaPicker : public rpc::ReplicaPicker<RemoteTabletServer> { Nit: maybe "using rpc::ReplicaPicker" at the top? Line 182: MetaCacheReplicaPicker(KuduClient* client, Does the client need to outlive the picker? Why not pass a shared_ptr to the client? Line 183: const scoped_refptr<MetaCache>& meta_cache, You can get the MetaCache from the client itself, so why pass them separately? Line 244: followers_copy = followers_; Does it matter that the contents of followers_ may have changed since L222? I don't think it does, but I wanted to ask to be sure. Line 281: Bind(&MetaCacheReplicaPicker::LookUpTabletCb, Unretained(this), callback, deadline)); Are copies made of 'callback' and 'deadline'? If not, the caller must ensure that they remain valid until the callback is called. Line 338: Separate with empty lines. Line 1000: tablet_id = rpc.ops()[0]->tablet->tablet_id(); The RPC itself has the tablet_id too; you could use it directly on L1017. http://gerrit.cloudera.org:8080/#/c/3017/4/src/kudu/rpc/replicated_rpc.h File src/kudu/rpc/replicated_rpc.h: Line 45: class ReplicaPicker : public RefCountedThreadSafe<ReplicaPicker<Replica>> { Why must every ReplicaPicker inherit from RefCountedThreadSafe? Also, please add a class comment and separate the methods with empty lines. Line 49: // Picks the leader replica among the replicas. Should mention how the deadline figures into it. Also, if the callback is invoked on a reactor thread, may want to mention that too. Line 50: // If this could find the leader it calls the callback with Status::OK() and Nit: "If the leader was found, it calls..." Line 52: // with 'status' set to the failure reason and 'replica' set to 'nullptr'. Nit: can just say null instead of 'nullptr'. Line 54: // Marks a replica as failed/unacessible. What effect does the status have? Line 56: // Marks a replicas as not the leader. Nit: replica Line 58: // Marks a replica as not part of the config. Nit: not part of the Raft configuration (otherwise you're just repeating the name of the method without adding new information). Line 60: private: Don't need this. -- To view, visit http://gerrit.cloudera.org:8080/3017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id43db316606cb807ec0019c79b3bdf76fa509fe5 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
