-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Retry-After hint #1916
Add Retry-After hint #1916
Conversation
Great work @gautam1168! I've just tried your change by doing sudo systemctl stop postgresql
curl -I localhost:3000/projects
HTTP/1.1 503 Service Unavailable
Date: Tue, 17 Aug 2021 16:54:18 GMT
Server: postgrest/8.0.0 (b3899e7)
Retry-After: 16
Content-Type: application/json; charset=utf-8
Ideally we'd be able to add a test, but our So this should be good. Don't forget to add an entry to the CHANGELOG! |
Just noted something that perhaps should be handled in another PR: the connection worker delay is not being respected. On v7, it used to be that all requests would get rejected with 503 while the connection worker is running. But on v8.0, the connection recovery is done whenever a new request comes in: curl -I localhost:3000/projects
HTTP/1.1 503 Service Unavailable
Date: Tue, 17 Aug 2021 17:15:12 GMT
Server: postgrest/8.0.0 (b3899e7)
Retry-After: 32
Content-Type: application/json; charset=utf-8
sudo systemctl start postgresql
## just a second after
curl -I localhost:3000/projects
HTTP/1.1 200 OK
Date: Tue, 17 Aug 2021 17:15:46 GMT
Server: postgrest/8.0.0 (b3899e7)
Content-Range: 0-58/*
Content-Location: /projects
Content-Type: application/json; charset=utf-8
## postgrest is already recovered
## The connection worker will report the reconnection later(after 32 sec):
## 17/Aug/2021:12:16:05 -0500: Connection successful
## 17/Aug/2021:12:16:05 -0500: Config re-loaded
## 17/Aug/2021:12:16:06 -0500: Schema cache loade This is probably a side effect of doing #1559. |
@steve-chavez I have added an entry to CHANGELOG.md in the unreleased fixes section. Should it be in the Added section? Regarding connection worker delay not being respected. I am assuming you will be opening another issue to handle that and maybe I will give that a try too. But next want to try to add the limit=0 feature request from another issue. |
30979d3
to
8908466
Compare
I figured since its not a bug it should be in the I will figure out how to rebase against the upstream repository and resolve this conflict. |
8908466
to
4e3c192
Compare
This fixes issue PostgREST#1817, Add Retry-After hint when in recovery mode
4e3c192
to
f613201
Compare
@steve-chavez I have resolved the conflict and squashed my changes to a single commit. I have also added a better commit message and included the issue number that is fixed in the description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Co-authored-by: Remo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Merging 🚀
Fixes issue 1878