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

Allow reloading with non POST methods #5343

Closed
eirc opened this Issue Mar 12, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@eirc
Copy link

eirc commented Mar 12, 2019

The /-/reload endpoint only works with the POST method. It would possibly make more sense to allow other methods too like PUT.

router.Post("/-/reload", h.reload)

The reasoning is that POST by definition produces different side effects each time it is run (like create new resources under REST) compared to PUT which you can supposedly safely fire multiple times.

Technically my problem is when trying to reload prometheus with a python program and urllib3 refuses to retry POST requests when there's a read error (specifically) since resending a POST when the server may have already received is a no no although it's not really a problem in config reloading.

https://github.com/urllib3/urllib3/blob/2a0957ea27e966166f81f88693af8e2f87d19fb6/src/urllib3/util/retry.py#L97-L104

PS: I'm also suggesting to just add the PUT instead of replacing the POST just for compatibility reasons, I'm sure many would take issue with such an API change.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 12, 2019

Longer term we plan on switching this to PUT for XSRF reasons, so supporting both now would make sense.

Technically my problem is when trying to reload prometheus with a python program and urllib3 refuses to retry POST requests when there's a read error

Is that supported by the RFCs?

@eirc

This comment has been minimized.

Copy link
Author

eirc commented Mar 12, 2019

TBH I have no idea what RFCs say about this but it does make sense IMHO.

@bharaththiruveedula

This comment has been minimized.

Copy link
Contributor

bharaththiruveedula commented Mar 16, 2019

@brian-brazil can I work on this?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Mar 16, 2019

Sure

@Danielv123

This comment has been minimized.

Copy link

Danielv123 commented Mar 26, 2019

With the PR merged, this issue should be considered for closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.