-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: staging
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package co.novu.common.rest; | ||
|
||
import java.io.IOException; | ||
import java.util.UUID; | ||
|
||
import okhttp3.Interceptor; | ||
import okhttp3.Request; | ||
import okhttp3.Response; | ||
|
||
public class IdempotencyKeyInterceptor implements Interceptor{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
@Override | ||
public Response intercept(Chain chain) throws IOException { | ||
Request request = chain.request(); | ||
Response response = null; | ||
|
||
Request requestWithIdempotencyKey = request.newBuilder() | ||
.header("Idempotency-Key", generateIdempotencyKey()) | ||
.build(); | ||
response = chain.proceed(requestWithIdempotencyKey); | ||
return response; | ||
} | ||
|
||
private String generateIdempotencyKey() { | ||
UUID uuid = UUID. randomUUID(); | ||
return uuid.toString(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
package co.novu.common.rest; | ||
|
||
import co.novu.common.base.NovuConfig; | ||
import co.novu.common.contracts.IRequest; | ||
import java.io.IOException; | ||
import java.util.Map; | ||
|
||
import com.google.gson.Gson; | ||
import com.google.gson.GsonBuilder; | ||
|
||
import co.novu.common.base.NovuConfig; | ||
import co.novu.common.contracts.IRequest; | ||
import lombok.RequiredArgsConstructor; | ||
import okhttp3.OkHttpClient; | ||
import okhttp3.Request; | ||
|
@@ -12,9 +16,6 @@ | |
import retrofit2.Retrofit; | ||
import retrofit2.converter.gson.GsonConverterFactory; | ||
|
||
import java.io.IOException; | ||
import java.util.Map; | ||
|
||
@RequiredArgsConstructor | ||
public class RestHandler { | ||
|
||
|
@@ -35,6 +36,14 @@ public Retrofit buildRetrofit() { | |
.build(); | ||
return chain.proceed(request); | ||
}).addInterceptor(new HttpLoggingInterceptor().setLevel(HttpLoggingInterceptor.Level.BASIC)); | ||
|
||
if(novuConfig.isEnableRetry()) { | ||
clientBuilder.addInterceptor(new RetryInterceptor(novuConfig.getMaxRetries(), novuConfig.getMinRetryDelayMillis() , novuConfig.getMaxRetryDelayMillis() , novuConfig.getInitialRetryDelayMillis())); | ||
} | ||
|
||
if(novuConfig.isEnableIdempotencyKey()) { | ||
clientBuilder.addInterceptor(new IdempotencyKeyInterceptor()); | ||
} | ||
Comment on lines
+44
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
Gson gson = new GsonBuilder() | ||
.setLenient() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
package co.novu.common.rest; | ||
|
||
import java.io.IOException; | ||
|
||
import okhttp3.Interceptor; | ||
import okhttp3.Request; | ||
import okhttp3.Response; | ||
|
||
public class RetryInterceptor implements Interceptor{ | ||
|
||
private final int maxRetries; | ||
private final int minRetryDelayMillis; | ||
private final int maxRetryDelayMillis; | ||
private final int initialRetryDelayMillis; | ||
|
||
public RetryInterceptor(int maxRetries, int minRetryDelayMillis, int maxRetryDelayMillis, int initialRetryDelayMillis) { | ||
this.maxRetries = maxRetries; | ||
this.minRetryDelayMillis = minRetryDelayMillis; | ||
this.maxRetryDelayMillis = maxRetryDelayMillis; | ||
this.initialRetryDelayMillis = initialRetryDelayMillis; | ||
} | ||
|
||
@Override | ||
public Response intercept(Chain chain) throws IOException { | ||
Request request = chain.request(); | ||
Response response = null; | ||
IOException lastException = null; | ||
|
||
for (int retry = 0; retry < maxRetries; retry++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use a while loop instead. Something like this 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 |
||
try { | ||
response = chain.proceed(request); | ||
if (response.isSuccessful()) { | ||
return response; // Request was successful, no need to retry | ||
} | ||
} catch (IOException e) { | ||
lastException = e; | ||
} | ||
|
||
try { | ||
int retryDelay; | ||
if (retry == 0) { | ||
retryDelay = initialRetryDelayMillis; | ||
} else { | ||
retryDelay = (int) (initialRetryDelayMillis * Math.pow(2, retry - 1)); | ||
} | ||
retryDelay = Math.max(retryDelay, minRetryDelayMillis); | ||
retryDelay = Math.min(retryDelay, maxRetryDelayMillis); | ||
Thread.sleep(retryDelay); | ||
} catch (InterruptedException ignored) { | ||
Thread.currentThread().interrupt(); | ||
} | ||
} | ||
|
||
// If all retries failed, throw the last exception | ||
if (lastException != null) { | ||
throw lastException; | ||
} | ||
|
||
return response; | ||
} | ||
|
||
} |
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 these should be false by default