Add support for gRPC response trailers in EPP#2510
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
size/L
| } | ||
|
|
||
| func (s *StreamingServer) finishResponse(ctx context.Context, reqCtx *RequestContext, body []byte) error { | ||
| if reqCtx.ResponseComplete { |
There was a problem hiding this comment.
add a comment pls on what this means, iiuc it means we already completed the response and so we don't want to execute finishResponse again, right?
| } | ||
| case *extProcPb.ProcessingRequest_ResponseTrailers: | ||
| // This is currently unused. | ||
| // For HTTP, the response trailer is not sent. Thus, it won't achieve this case. |
There was a problem hiding this comment.
even if it was sent in the http case and this logic gets executed, we should be ok, right?
There was a problem hiding this comment.
yes, because we only set reqCtx.ResponseComplete to true in the new finishResponse. and the first early return check for finishResponse makes it idempotent in the whole request lifecycle
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, zetxqx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
This line fails in integration tests: gateway-api-inference-extension/test/integration/util.go Lines 479 to 481 in 9c00cd7 
/retest |
|
can we increase the timeout to 10s pls? |
* processing respTrailers for gRPC request * polish comments
…y-api-inference-extension#2510) * processing respTrailers for gRPC request * polish comments
3 participants
What type of PR is this?
What this PR does / why we need it:
This PR adds
ResponseTrailershandling to the EPP ExtProc server to properly support gRPC requests. gRPC over HTTP/2 relies on response trailers to signal the end of a response, rather than justEndOfStreamon the response body.for gRPC over HTTP2 resposne endOfStream see: Ref
Which issue(s) this PR fixes:
pre-req for #2166
Does this PR introduce a user-facing change?: