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

Backoff time #411

Merged
merged 17 commits into from
Jun 15, 2022
Merged

Backoff time #411

merged 17 commits into from
Jun 15, 2022

Conversation

merkys
Copy link
Member

@merkys merkys commented Jun 1, 2022

Fixes #409 by introducing request_delay, an optional field in meta of any response to ask the client to wait specified number of seconds (float) before issuing subsequent request.

@merkys merkys requested a review from rartino June 1, 2022 08:58
@ml-evs ml-evs added the type/proposal Proposal for addition/removal of features. May need broad discussion to reach consensus. label Jun 1, 2022
@dwinston
Copy link
Contributor

dwinston commented Jun 1, 2022

Given that HTTP is assumed as the API access protocol, we can also leverage the standard Retry-After response header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After.

@merkys
Copy link
Member Author

merkys commented Jun 1, 2022

@dwinston fair point. I will adjust the proposal to add Retry-After.

rartino
rartino previously approved these changes Jun 2, 2022
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Consider moving the error code suggestion as discussed.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

A useful addition to the spec!

Do we prefer request_delay instead of just using the same nomenclature as retry_after? One minor formatting comment below.

optimade.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Thanks for the work with this. Some suggestions to consider.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated
Comment on lines 497 to 498
An implementation MAY respond with :http-error:`429 Too Many Requests` error if a client refuses to respect the indicated delay.
In that case the response SHOULD contain ``Retry-After` HTTP header <https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After>`__ to instruct the client to wait before retrying.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this belongs elsewhere - the SHOULD-level requirement that server return the Retry-After http header in a 429 response should apply regardless of whether the request_delay field is used or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this has already been discussed, and we agreed it should be moved elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

@merkys Do you intend to make this change? (Or do you want someone to help?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@rartino yes, I intend to do so, but it would help me a lot to know where exactly should I move this text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, will move soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

But do we really need this text then? Currently, "HTTP Response Status Codes" and "HTTP Response Headers" are pretty concise, only providing pointers to HTTP and JSON API standards. Do we want to duplicate that locally? HTTP 429 and HTTP Retry-After seem to be pretty standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say no, but there were others pushing for this info. I think the primary problem I have with just leaving what you have as it is, is that it adds a requirement on HTTP headers in a section that is, in a sense, not strinctly tied to HTTP as the transmission protocol. I'll try to suggest a requirement-free version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I have accepted it. I like that HTTP 429 and Retry-After are now in an implementation note. This way OPTIMADE specification does not add specific interpretation on top of HTTP standards.

Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
merkys and others added 2 commits June 7, 2022 08:49
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
@merkys
Copy link
Member Author

merkys commented Jun 7, 2022

@ml-evs

Do we prefer request_delay instead of just using the same nomenclature as retry_after? One minor formatting comment below.

I would indeed prefer sticking to the same nomenclature, but "retry" part does not sound right here. I would expect "retry" to mean "issue the exact same request". Maybe next_request_after or just request_after sounds better?

JPBergsma
JPBergsma previously approved these changes Jun 10, 2022
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

This PR seems good to me.

@rartino
Copy link
Contributor

rartino commented Jun 14, 2022

@ml-evs, @merkys

Do we prefer request_delay instead of just using the same nomenclature as retry_after? One minor formatting comment below.

I would indeed prefer sticking to the same nomenclature, but "retry" part does not sound right here. I would expect "retry" to mean "issue the exact same request". Maybe next_request_after or just request_after sounds better?

As a remark: I first though the http header retry_after was not appropriate at all outside delivering an http errors. However, digging a bit in the RFCs made me realize that while the latest RFC allows it to be used also the way we say. The reason for the name is thus "historical debt".

Nevertheless, I agree with @merkys to rather just use a more helpful term.

ml-evs
ml-evs previously approved these changes Jun 14, 2022
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Fine by me!

optimade.rst Outdated Show resolved Hide resolved
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
@merkys merkys dismissed stale reviews from ml-evs and JPBergsma via 924283d June 15, 2022 06:40
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Good to go!

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I think I slightly preferred the SHOULD/MAY formulation, but this still looks good to me.

@merkys merkys merged commit ece4cad into Materials-Consortia:develop Jun 15, 2022
@merkys merkys deleted the backoff-time branch June 15, 2022 08:48
@ml-evs ml-evs added this to the v1.2 milestone Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal Proposal for addition/removal of features. May need broad discussion to reach consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backoff time
5 participants