Dan Burkert 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 befo Done 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 The latter, and I've added a test to make sure it continues to work. Line 510: delete transfer; > we don't need to pop_front() before deleting it? Done Line 511: return; > why not 'continue' on to the next call in the queue here? Done 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 Done 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 Done Line 97: const function<void(const unordered_set<uint32_t>&)>& f) { > why this weird functional interface instead of just returning a const refer Done 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 Done 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* Done 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 b Done -- 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
