pjfanning commented on PR #862:
URL: https://github.com/apache/pekko-grpc/pull/862#issuecomment-5558977663
## Review notes
Verified in a clean worktree on the PR head (`743a5c44`): `runtime/compile`
(2.13 and 3.3.8), `runtime/mimaReportBinaryIssues`, `runtime/test` (179 pass),
`interop-tests/test` (255 pass), `plugin-tester-scala/compile` +
`plugin-tester-java/compile` (so the generated code from the changed Twirl
templates compiles), `benchmarks/compile`, and `scalafmtCheck` on all touched
modules. All green — I did not find a functional break. The notes below are the
substantive points.
### Medium
**1.
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala:146`
— the exception handler is now built on every unary request, including the
happy path.**
`GrpcExceptionHandler.from(eHandler(system.classicSystem))` is constructed
unconditionally. Previously the recovery `PartialFunction` was only built on
the error branch — `case Some(Success(out)) => marshalUnaryResponse(...)`
allocated nothing. `from` calls `eHandler(system)`, then
`defaultMapper(system)`, then `.orElse(...).andThen(...)`: 4+ allocations per
successful call.
This looks like a plausible cause of the regressions in this PR's own
numbers (`GrpcMarshallingBenchmark.handleUnaryStrict` 53.4M → 23.0M ops/s,
`syntheticUnaryStrictRequestProcessing` 29.9M → 24.6M). The PF depends only on
`eHandler`, `system` and `writer`, all fixed per handler, so it can be hoisted
out of the per-request path.
**2.
`runtime/src/main/scala/org/apache/pekko/grpc/scaladsl/GrpcMarshalling.scala:145`
— response marshalling moved to `ExecutionContext.parasitic`.**
When the service implementation's `Future` completes asynchronously,
`marshal[Out]` (full protobuf serialization plus response construction) now
runs inline on whatever thread calls `Promise.success` — a JDBC pool thread, an
outbound client's Netty event loop, or an actor's dispatcher thread if the
implementation completes a promise from inside `receive`. It previously ran on
`mat.executionContext`. For large response messages that is real work stolen
from a foreign thread, and it happens while any lock the completer holds is
still held.
The same applies to the `recoverWith` on the next line, which runs
`GrpcExceptionHandler.defaultMapper` — including `Logging(system, …)`
construction — on that thread.
**3.
`runtime/src/main/scala/org/apache/pekko/grpc/ProtobufSerializer.scala:39` —
the new public extension point exposes an internal type.**
`ProtobufFrameSerializer` is promoted from `private[grpc]` to public
`@ApiMayChange`, but it extends
`GrpcProtocol.DeferredDataFrame.DeferredDataWriter[T]`, which is nested inside
`@InternalApi object GrpcProtocol`, and both abstract members
(`serializedSize`, `serializeTo`) are inherited from that internal type. Anyone
implementing the hook has to reference a type we declare internal and unstable.
If this is meant to be a supported optimisation hook, `DeferredDataWriter`
should probably move out of `GrpcProtocol`, or the two methods be declared
directly on `ProtobufFrameSerializer`.
### Low
**4.
`runtime/src/main/scala/org/apache/pekko/grpc/javadsl/GrpcMarshalling.scala:129`
— binary break against 2.0.0-M2 that MiMa will not catch.**
`handleUnaryResponse` and both `handleUnaryFailure` overloads are removed,
and the Java codegen no longer emits calls to them. Java handlers generated by
2.0.0-M2 and re-run against this build will fail with `NoSuchMethodError` at
request time. MiMa misses it because `mimaCompareVersion` is `1.0.2`
(`build.sbt:102`) and these methods did not exist in 1.0.2.
Milestone-to-milestone this is probably acceptable, but it is worth a changelog
note since it forces regeneration.
**5.
`runtime/src/main/scala/org/apache/pekko/grpc/internal/GrpcProtocolNative.scala:71`
— the parameter widening loses an invariant.**
`encodeDataToResponse` now takes an `OutboundFrame` and derives the body via
`encodeFrame(codec, frame).data`. `TrailerFrame` is an `OutboundFrame`, and for
it `encodeFrame` returns `LastChunk`, whose `.data` is `ByteString.empty` — so
passing a trailer frame silently produces a 200 response with an empty body and
no trailers rather than failing. The old `ByteString` parameter made that
unrepresentable. `GrpcProtocolWeb.scala:51` has the mirror problem: a
`TrailerFrame` would be encoded as a trailer frame *plus* the appended trailer
frame. Narrowing the parameter to a data-frame type, or matching and rejecting
`TrailerFrame`, would restore the guarantee.
**6. Leftover debug output in the two new tests.**
`runtime/src/test/scala/org/apache/pekko/grpc/javadsl/GoogleProtobufSerializerSpec.scala:41`
and
`runtime/src/test/scala/org/apache/pekko/grpc/scaladsl/ScalapbProtobufSerializerSpec.scala:42`
still have `println(serialized.mkString(","))` /
`println(frame.mkString(","))` in the "serialize into buffer at offset" tests.
They fire on every `runtime/test` run.
**7.
`runtime/src/test/scala/org/apache/pekko/grpc/javadsl/GrpcMarshallingSpec.scala:59`
— coverage loss.**
The deleted `"recover a failed unary response stage"` test was the only unit
test asserting that a failed unary `CompletionStage` is turned into a
`grpc-status` response in the Java DSL. The behaviour itself is still correct —
`GrpcExceptionHandler.standard` unwraps `CompletionException` — but it is now
only covered indirectly, via the generated
`unmarshal(...).thenCompose(...).thenApply(...).exceptionally(GrpcExceptionHandler.standard)`
shape. Worth replacing with an equivalent test against that shape.
**8.
`runtime/src/main/scala/org/apache/pekko/grpc/internal/GrpcProtocolWeb.scala:43`
— parameter typo `frane`.**
Used consistently at line 47, so it compiles and behaves correctly.
Separately: the web variant's `encodeFrame` / `encodeFrameToBytes` still take
`Frame` rather than `OutboundFrame`, unlike the native variant — worth aligning
now that the inbound/outbound split exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
