LGTM

Just a few drive-by comments, all minor cosmetic issues.

Thanks for doing this!

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

include/grpcpp/impl/codegen/server_context_impl.h, line 182 at r1 (raw file):

  /// connection reset, HTTP/2 parameter configuration (e.g., max message size,
  /// max connection age), etc. It does NOT include failure due to a non-OK
  /// status return, including Status::CANCELLED.

Maybe say "from the server application's request handler", just to be completely clear?

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

  grpc_closure* send_initial_metadata_finished = nullptr;
  grpc_metadata_batch* send_trailing_metadata = nullptr;
  bool* sent_trailing_metadata_op = nullptr;

Suggest calling this trailing_metadata_sent.

src/core/lib/transport/transport.h, line 250 at r1 (raw file):

    // as  a cancellation (since the stream was write-closed before status could
    // be delivered).
    bool* sent_trailing_metadata = nullptr;

This being only one character off of the previous field is likely to cause confusion. How about just calling this sent, and relying on the fact that it's inside of the send_trailing_metadata struct for the additional context?

And thanks again for the review!

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @markdroth)

include/grpcpp/impl/codegen/server_context_impl.h, line 182 at r1 (raw file):

Done.

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

So I'd prefer to keep this to indicate that this is a field of the op. The name trailing_metadata_sent sounds like a bool rather than a bool* . That said, I'd be fine with a different name. Maybe for later re-consideration?

src/core/lib/transport/transport.h, line 250 at r1 (raw file):

Done.

Please resolve the one remaining comment before merging. Thanks!

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

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

We don't use the _op suffix for any of the other fields that point into the grpc_transport_stream_op_batch_payload struct. Another example of a bool* field is trailing_metadata_available.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

I should have also said: there's already a field called sent_trailing_metadata which is a bool for CHTTP2's internal control, so trailing_metadata_sent would be super-confusing.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

How about sent_trailing_metadata_notify ?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vjpai)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

This sounds like an argument for a better naming scheme for all of these members, to clarify which ones are for returning values and which ones are for internal state. But I recognize that that may be a larger effort that should be done independently of this PR. Maybe just add a TODO for now?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

TODO sounds good for now. Thanks!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

TODONE and assigned to @yashykt as the chttp2 owner

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)

src/core/ext/transport/chttp2/transport/internal.h, line 536 at r1 (raw file):

Done.

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved