David Ribeiro Alves has posted comments on this change. Change subject: Add an all-virtual ServerPicker class and a meta cache backed implementation ......................................................................
Patch Set 4: (18 comments) 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 Done Line 14: logic simples (less callbacks) and makes the replica picking code possibly reusable. > Nit: simpler, not simples Done 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). Done Line 180: class MetaCacheReplicaPicker : public rpc::ReplicaPicker<RemoteTabletServer> { > Nit: maybe "using rpc::ReplicaPicker" at the top? Done Line 182: MetaCacheReplicaPicker(KuduClient* client, > Does the client need to outlive the picker? Why not pass a shared_ptr to th the client can't outlive it. this is build from Batcher which also has a raw pointer to KuduClient. It was the same where this code came from and I don't want to go down the rabbit hole of sorting lifecycle issues. Line 183: const scoped_refptr<MetaCache>& meta_cache, > You can get the MetaCache from the client itself, so why pass them separate cuz in a followup patch I want to refactor the RemoteTabletServer::InitProxy to not take the client and just take the dns resolver. seems silly to be passing a raw pointer of the client around if all we need is the ability to resolve dns addresses. Line 244: followers_copy = followers_; > Does it matter that the contents of followers_ may have changed since L222? not really, on l222 we're checking if the leader was marked as follower and only reach here if it has. The worst that can happen here is that the leader has been somehow put back, in which isn't problematic. Line 281: Bind(&MetaCacheReplicaPicker::LookUpTabletCb, Unretained(this), callback, deadline)); > Are copies made of 'callback' and 'deadline'? If not, the caller must ensur the caller does, yes, as it had to do before, again would prefer not to be futzing with lifecycle. Line 338: > Separate with empty lines. Done 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. You mean add an accessor? Done 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? it's mean to be shared across rpc's (see follow up patches) so it makes sense for it to be able remain alive on its own. Are you're talking about the fact that the MetaCacheReplicaPicker ends up being owned by RemoteTablet and that we keep raw pointers to it all over? Line 49: // Picks the leader replica among the replicas. > Should mention how the deadline figures into it. Also, if the callback is i Re the deadline: Done Re where the callback gets executed: don't want to have to specify where the callback gets executed since this is only an interface. For instance we might want to call the callback inline if we already know who the leader is and it is initialized. Line 50: // If this could find the leader it calls the callback with Status::OK() and > Nit: "If the leader was found, it calls..." Done Line 52: // with 'status' set to the failure reason and 'replica' set to 'nullptr'. > Nit: can just say null instead of 'nullptr'. removed that part, since it's actually not true. Line 54: // Marks a replica as failed/unacessible. > What effect does the status have? depends on what's behind this. The RemoteTablet makes 'status' the failure reason. Line 56: // Marks a replicas as not the leader. > Nit: replica Done Line 58: // Marks a replica as not part of the config. > Nit: not part of the Raft configuration (otherwise you're just repeating th changed to a more generic MarkResourceNotFound Line 60: private: > Don't need this. Done -- 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
