Todd Lipcon has posted comments on this change. Change subject: service_queue: use futex for waking waiting rpc threads ......................................................................
Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/2940/5/src/kudu/rpc/service_queue.cc File src/kudu/rpc/service_queue.cc: Line 61: if (call != nullptr) { > Can consumer->Reset() put before this line, so it's out of spinlock? But I Done http://gerrit.cloudera.org:8080/#/c/2940/5/src/kudu/rpc/service_queue.h File src/kudu/rpc/service_queue.h: Line 27: #include "kudu/gutil/atomicops.h" > Nit: resort. Done Line 183: return call_; > Why don't we need to nullify call_ before returning like we did before? that gets done by calling Reset() now. Otherwise we'd have to duplicate the nullification code in all three of the return paths from this function. Line 191: DCHECK_EQ(old, NOT_BLOCKED); > Seems a little self-explanatory, given the semantics of CAS. I don't think so -- what we're asserting here is that the CAS succeeded, and 'old' wasn't some other value like SLEEPING or junk. Line 205: enum State { > Would be nice to see a few comments explaining the significance of Consumer Done -- To view, visit http://gerrit.cloudera.org:8080/2940 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ice7bb703d643b722f732f4c4ebc391d25796d3df Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Binglin Chang <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
