I'm providing a few drive-by comments here, but I'll let @karthikravis do the main review, since he's the component owner for the server code on the cloud side.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @vjpai)

src/core/lib/surface/server.cc, line 139 at r1 (raw file):

struct call_data;

class RequestMatcherInterface {

Please document what this interface is for.

src/core/lib/surface/server.cc, line 142 at r1 (raw file):

 public:
  virtual ~RequestMatcherInterface() {}
  virtual void ZombifyPending() = 0;

Please document what each method is for along with any applicable parameters, return values, and pre- and post-conditions.

src/core/lib/surface/server.cc, line 258 at r1 (raw file):

  registered_method* registered_methods;
  /** one request matcher for unregistered methods */
  // TODO(vjpai): Convert to a std::unique_ptr once grpc_server has a real

I don't quite understand the condition here. The real implementation has a ctor and dtor, and that's what would need to be used here, right? What's stopping this from being a unique_ptr<> immediately?

Actually, isn't the real problem here that grpc_server is a C struct instead of a C++ object, so C++ data members won't be destructed properly? Maybe we should change this to be a C++ object while we're in here.

src/core/lib/surface/server.cc, line 377 at r1 (raw file):

    }
  }
  void ZombifyPending() override {

Please add a blank line between methods.

src/core/lib/surface/server.cc, line 391 at r1 (raw file):

  }
  void KillRequests(grpc_error* error) override {
    for (size_t i = 0; i < requests_per_cq_.size(); i++) {

for (LockedMultiProducerSingleConsumerQueue& queue : requests_per_cq_) {

src/core/lib/surface/server.cc, line 487 at r1 (raw file):

  call_data* pending_head_ = nullptr;
  call_data* pending_tail_ = nullptr;
  std::vector<LockedMultiProducerSingleConsumerQueue> requests_per_cq_;

Would it make sense to use absl::InlinedVector<LockedMultiProducerSingleConsumerQueue, 1> here, to avoid an allocation in the presumably common case where there is only one CQ?

Thanks again for the comments! This is ready for re-review as I think I've addressed what you asked for.

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @markdroth)

src/core/lib/surface/server.cc, line 139 at r1 (raw file):

Done.

src/core/lib/surface/server.cc, line 142 at r1 (raw file):

Done.

src/core/lib/surface/server.cc, line 258 at r1 (raw file):

As we discussed, let's do this as part of the C++ification of grpc_server (separate PR)

src/core/lib/surface/server.cc, line 377 at r1 (raw file):

Done.

src/core/lib/surface/server.cc, line 391 at r1 (raw file):

We need the value "i" as an argument to the fail_call function below.

src/core/lib/surface/server.cc, line 487 at r1 (raw file):

It's worth considering, but not sure if it's a win because 1 CQ may not be a common case - sync servers by default have 1 notification CQ per core; callback API will use a different RequestMatcher specified in the next PR, and only async API might be affected (and we're not necessarily spending effort on its performance)

Looks good. Feel free to merge after fixing the typo.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vjpai)

src/core/lib/surface/server.cc, line 160 at r3 (raw file):

  // How many request queues are supported by this matcher. This is an abstract
  // concept that essentially mapps to gRPC completion queues.

s/mapps/maps/