Adar Dembo 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"? Also, should be ": the RequestTracker." 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. Perhaps you could summarize a few key design considerations in the commit description? Alternatively, do that in request_tracker.h, since I see the class comment is spartan. 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? Line 35: return Status::ServiceUnavailable("Could not create a new sequence number." Nit: start status messages with lower-case letters. Below too. 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 Line 35: class RequestTracker : public RefCountedThreadSafe<RequestTracker> { Can you document why the RequestTracker needs to be shared ownership? Is it possible to do that without talking too much about other rpc modules? Line 38: explicit RequestTracker(const std::string& client_id); Do callers need to guarantee anything about client_id? If so, please document that here. 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 not clear where the client id is "copied" from. Nor is it clear who is passing it where. I think you can omit this explanation altogether. If you do keep it, "havind" -> "having". Line 64: // The (ordered) set of incomplete RPCs. Nit: separate fields with blank lines. -- 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
