Skip to content
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

Merged
merged 2 commits into from Aug 18, 2021
Merged

Add Retry-After hint #1916

merged 2 commits into from Aug 18, 2021

Conversation

gautam1168
Copy link
Contributor

Fixes issue 1878

@steve-chavez
Copy link
Member

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

Retry-After is now showing!

Ideally we'd be able to add a test, but our io-tests(in Python) need some work to support restarting/starting the postgres instance(#1766).

So this should be good. Don't forget to add an entry to the CHANGELOG!

@steve-chavez
Copy link
Member

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.

@gautam1168
Copy link
Contributor Author

gautam1168 commented Aug 18, 2021

@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.

@gautam1168
Copy link
Contributor Author

I figured since its not a bug it should be in the Added section and moved it.

I will figure out how to rebase against the upstream repository and resolve this conflict.

This fixes issue PostgREST#1817, Add Retry-After hint when in recovery mode
@gautam1168
Copy link
Contributor Author

@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.

Copy link
Member

@monacoremo monacoremo left a comment

Choose a reason for hiding this comment

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

Great work!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

LGTM! Merging 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants