Todd Lipcon has posted comments on this change. Change subject: rpc: call-level feature flags ......................................................................
Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 504: if (!transfer->TransferStarted()) { I think it's worth a comment explaining why we have to wait until just before we send it to check the flags (vs checking them before we enqueue) -- i.e. because we may have not yet done connection negotiation at enqueue time. Line 509: transfer->Abort(Status::NotSupported("Server is missing required RPC features")); should we worry that this call that never got sent still consumes a call ID? does the server verify that calls have increasing call IDs _with no gaps_? or just increasing? Line 510: delete transfer; we don't need to pop_front() before deleting it? Line 511: return; why not 'continue' on to the next call in the queue here? http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/inbound_call.h File src/kudu/rpc/inbound_call.h: Line 105: void RespondUnsupportedFeature(const vector<uint32_t>& unsupported_features); std::vector in header http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: Line 91: lock_guard<simple_spinlock> l(&lock_); per below comment, I dont think you really need synchronization here. does TSAN complain if you don't have it? Line 97: const function<void(const unordered_set<uint32_t>&)>& f) { why this weird functional interface instead of just returning a const reference to the member in a normal getter? RpcController's _setup_ isn't meant to be thread-safe. http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: Line 151: void WithRequiredServerFeatures( see comment in .cc http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: Line 140: // NOTE: these are evolving features at the level of the application, not the these are *for* http://gerrit.cloudera.org:8080/#/c/2239/3/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: Line 112: std::set<RpcFeatureFlag> required_features, doc (also include a note about why this goes with the transfer instead of being checked earlier) -- To view, visit http://gerrit.cloudera.org:8080/2239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I46e46bcf80b93fc6cce50f37dd71a6e6539047f8 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
