-
Notifications
You must be signed in to change notification settings - Fork 37
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
Backoff time #411
Conversation
Given that HTTP is assumed as the API access protocol, we can also leverage the standard |
@dwinston fair point. I will adjust the proposal to add |
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.
Consider moving the error code suggestion as discussed.
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.
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.
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.
Thanks for the work with this. Some suggestions to consider.
optimade.rst
Outdated
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. |
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.
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.
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.
Right, this has already been discussed, and we agreed it should be moved elsewhere.
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.
@merkys Do you intend to make this change? (Or do you want someone to help?)
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.
@rartino yes, I intend to do so, but it would help me a lot to know where exactly should I move this text.
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.
I think the only place that makes sense is https://github.com/Materials-Consortia/OPTIMADE/blob/develop/optimade.rst#http-response-status-codes
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.
Ah, right, will move soon.
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.
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.
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.
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.
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.
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 <[email protected]>
Co-authored-by: Rickard Armiento <[email protected]>
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 |
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.
This PR seems good to me.
As a remark: I first though the http header Nevertheless, I agree with @merkys to rather just use a more helpful term. |
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.
Fine by me!
Co-authored-by: Rickard Armiento <[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.
Good to go!
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.
I think I slightly preferred the SHOULD/MAY formulation, but this still looks good to me.
Fixes #409 by introducing
request_delay
, an optional field inmeta
of any response to ask the client to wait specified number of seconds (float) before issuing subsequent request.