Skip to content

Forward HTTP status code for sync requests #375

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

yunfeng-scale
Copy link
Contributor

@yunfeng-scale yunfeng-scale commented Nov 14, 2023

Pull Request Summary

We used to always forward with 200s ignoring downstream HTTP codes, now forwarding the code as well

Test Plan and Usage Guide

unit tests and tested with echo server to make sure 500 status was forwarded

curl -X POST localhost:5000/predict -d '{"args":{}}' -H "content-type:application/json" -vv
Note: Unnecessary use of -X or --request, POST is already inferred.
* processing: localhost:5000/predict
*   Trying [::1]:5000...
* Connected to localhost (::1) port 5000
> POST /predict HTTP/1.1
> Host: localhost:5000
> User-Agent: curl/7.68.0
> Accept: */*
> content-type:application/json
> Content-Length: 11
> 
< HTTP/1.1 500 Internal Server Error
< date: Tue, 14 Nov 2023 21:23:58 GMT
< server: uvicorn
< content-length: 15
< content-type: application/json
< 
* Connection #0 to host localhost left intact
{"result":"{}"}

@yunfeng-scale yunfeng-scale requested a review from a team November 14, 2023 21:28
Copy link
Member

@yixu34 yixu34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Let's make an announcement before deploying this, because even though this is backwards compatible (schema-wise) and is more correct, it may surprise people if their alarms start going off.

@seanshi-scale
Copy link
Contributor

Nice! Let's make an announcement before deploying this, because even though this is backwards compatible (schema-wise) and is more correct, it may surprise people if their alarms start going off.

iiuc we'd either need to update forwarder images on everyone's existing deployments or have people update their deployments for the changes to be picked up?

@yunfeng-scale yunfeng-scale merged commit b319397 into main Nov 15, 2023
@yunfeng-scale yunfeng-scale deleted the yunfeng-http-forwarder-status-code branch November 15, 2023 00:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants