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

[Draft]: Added Exponential Retry Mechanism with Idempotency Headers #92

Draft
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

git-ashug
Copy link
Contributor

Related to #79

Hi @Cliftonz,
Please review this Draft PR as I am progressing on it.

  • As we were already using okhttp3 library, I have used the same library to implement the requirement as Interceptors.
  • As per the original requirement, I have added configuration options for enabling/disabling the Exponential Retry mechanism and Idempotency Key provisioning.
  • Provision to add Idempotency-Key in Header.
  • Configuration for Exponential Retry Mechanism as per novu-go SDK

Will keep you updated as I progress on this issue.

@unicodeveloper
Copy link
Contributor

@mayorJAY Please review

Comment on lines 21 to 22
private boolean enableRetry = true; // To enable/disable retry logic
private boolean enableIdempotencyKey = true; // To enable/disable idempotency key
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these should be false by default

Copy link
Collaborator

@mayorJAY mayorJAY left a comment

Choose a reason for hiding this comment

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

You can refer to this comment for more guidance on the requirement novuhq/novu#4462 (comment)

import okhttp3.Request;
import okhttp3.Response;

public class IdempotencyKeyInterceptor implements Interceptor{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't necessarily need a dedicated Interceptor for this. We can make it a dynamic header by using @ Headers for the POST and PATCH calls

Copy link
Contributor Author

@git-ashug git-ashug Oct 22, 2023

Choose a reason for hiding this comment

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

If we don't use interceptor for adding Idempotency header, do I need to add @Header annotation for POST and PATCH calls in all *Api classes? as that works on method level @mayorJAY

Response response = null;
IOException lastException = null;

for (int retry = 0; retry < maxRetries; retry++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use a while loop instead. Something like this while (!response.isSuccessful() && retry < maxRetries) {}
This will ensure that we're attempting the retrial only when the last call was not successful.

Also, note that the check for a successful call should be based on the the status codes specified here. You can have a utility function that does this check

Comment on lines +44 to +46
if(novuConfig.isEnableIdempotencyKey()) {
clientBuilder.addInterceptor(new IdempotencyKeyInterceptor());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The main reason why this can't be done is the fact that we provide the Retrofit instance lazily (see lines 27 to 29). Doing this would mean that every request will use the same value as the Idempotency-Key, which is not the goal

Copy link
Contributor

Choose a reason for hiding this comment

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

@mayorJAY I tried this interceptor and it is inputting new header value for each request made

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.

4 participants