David Ribeiro Alves has posted comments on this change. Change subject: Add a request tracker to track client rpc sequence numbers ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/3078/6//COMMIT_MSG Commit Message: Line 9: This adds a new component rpc::, the RequestTracker. This is inspired > Nit: instead of "rpc::" maybe "to the rpc subsystem"? Done Line 11: by Colin Lee et al. and is responsible for generating new sequence numbers for rpcs > Would be nice not to have to read the paper to understand the design. Perha enhanced the class comment http://gerrit.cloudera.org:8080/#/c/3078/6/src/kudu/rpc/request_tracker.cc File src/kudu/rpc/request_tracker.cc: Line 21: DEFINE_int32(rpc_max_in_flight_linearizable_rpcs, 512, > How did you choose 512 as the default value? this came from the paper, removed at todds request. Line 35: return Status::ServiceUnavailable("Could not create a new sequence number." > Nit: start status messages with lower-case letters. Below too. did we decide to change this? A quick search on the codebase reveals that the great majority of status messages have the first letter capitalized. http://gerrit.cloudera.org:8080/#/c/3078/6/src/kudu/rpc/request_tracker.h File src/kudu/rpc/request_tracker.h: Line 34: // This class Thread safe. > Nit: is thread safe Done Line 35: class RequestTracker : public RefCountedThreadSafe<RequestTracker> { > Can you document why the RequestTracker needs to be shared ownership? Is it it's hard to do that honestly, don't want to be too specific about lifetime as that only sets up the comment for staleness. Line 38: explicit RequestTracker(const std::string& client_id); > Do callers need to guarantee anything about client_id? If so, please docume not really. Line 58: // Copied here so that we can avoid havind to pass it around since we already need to pass : // the RequestTracker around. > Nit: this is a strange explanation. Without other patches in context, it's Done Line 64: // The (ordered) set of incomplete RPCs. > Nit: separate fields with blank lines. Done -- To view, visit http://gerrit.cloudera.org:8080/3078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I23201625ca02f244dc94205d88dabc01608de471 Gerrit-PatchSet: 6 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
