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 idempotency key #660

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

idelvall
Copy link

Adds an idempotency key to the headers sent by the requests and its retries so the server can use this header to avoid processing multiple times a non-idempotent implementation.

Idempotency keys are unique tokens that you submit as a request header, that guarantee that only one resource will be created regardless of how many times a request is sent to us.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable idea, but couldn't it just be another interceptor? Why does it need to be part of the retry logic? Also please add some tests.

@idelvall
Copy link
Author

idelvall commented Oct 16, 2023

This seems like a reasonable idea, but couldn't it just be another interceptor? Why does it need to be part of the retry logic? Also please add some tests.

Hi @johanbrandhorst, yes I considered that option, but my reasoning was that given that non-idempotent RPCs should not be retried, this is something that falls into the library scope.

Happy to add those tests if you think the change is worth it.

@johanbrandhorst
Copy link
Collaborator

Ah, you mean that the use of the retry interceptor implies that the request must be idempotent? I suppose I follow that logic.

@johanbrandhorst
Copy link
Collaborator

Yes, please do add the tests and fix the build, thanks

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

Successfully merging this pull request may close these issues.

2 participants