Todd Lipcon has posted comments on this change. Change subject: Add a request tracker to track client rpc sequence numbers ......................................................................
Patch Set 2: (6 comments) I'm not 100% sold on the ordering of this work vs other stuff. I thought your plan was to start with just time-based garbage collection on the server, and then later work on propagating these lower bounds? http://gerrit.cloudera.org:8080/#/c/3078/2//COMMIT_MSG Commit Message: Line 9: rpc::, the RequestTr typo Line 11: by Colin Lee et al. and is responsible for generating new sequence numbers for rpcs I think we should try to be clear where we mean "an RPC" to mean a specific call, vs a replicated RPC which actually consists of many different calls to different servers. Maybe we should use the term RRPC or something? Otherwise this is going to get confusing because each RPC also has an RPC call ID which is unique to that specific call, and different than its Replicated RPC sequence number http://gerrit.cloudera.org:8080/#/c/3078/2/src/kudu/rpc/request_tracker.cc File src/kudu/rpc/request_tracker.cc: Line 35: if (PREDICT_FALSE(incomplete_rpcs_.size() >= FLAGS_rpc_max_in_flight_linearizable_rpcs)) { what's the purpose of this bound? it's just to bound the size of the set? Shouldn't the set be fine until the millions of elements? http://gerrit.cloudera.org:8080/#/c/3078/2/src/kudu/rpc/request_tracker.h File src/kudu/rpc/request_tracker.h: Line 32: // for new rpcs and tracking the ongoing ones. per the comment on the commit message, I'm worried this is a little confusing to conflate sequence numbers here with the sequence numbers used within an RPC connection. This also should be clearer that this is a client side construct, and what the purpose is. The paper reference is nice, but when I'm in the middle of reading the code I don't really want to have to go find and read through a complex paper. Just a couple sentences like "this allows a client to occasionally propagate a lower bound on RRPC sequence numbers that it might retry in the future, so that servers can garbage-collect their replay caches" or whatever. Line 37: typedef int64_t SeqNo; hrm, not sure it's worth that much to abbreviate instead of SequenceNumber Line 67: simple_spinlock lock_; better to put the lock first before the fields it protects -- 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: 2 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
