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 delay before retrying same address #101

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@arhimondr
Contributor

arhimondr commented Nov 22, 2018

Retrying same address without a delay may lead to infinite loop

@arhimondr

This comment has been minimized.

Contributor

arhimondr commented Nov 22, 2018

CC: @dain

@arhimondr

This comment has been minimized.

Contributor

arhimondr commented Nov 22, 2018

Fixes: #100

@dain

I had a few suggestions. Let me know what you think.

@GuardedBy("this")
private final Set<A> attemptedAddresses = new LinkedHashSet<>();
private final Map<A, AtomicInteger> attemptedAddresses = new LinkedHashMap<>();

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

Why use an AtomicInteger if the access is always guarded by a synchronized lock on this.

This comment has been minimized.

@arhimondr

arhimondr Nov 27, 2018

Contributor

I was using AtomicInteger only because it is mutable. But you are right. I can simply use merge provided by the Map.

@@ -166,13 +184,14 @@ private synchronized void nextAttempt()
@Override
public void onSuccess(Object result)
{
attemptedAddresses.computeIfPresent(address, (key, value) -> new AtomicInteger(0));

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

This access needs a guard.

This comment has been minimized.

@arhimondr

arhimondr Nov 27, 2018

Contributor

Good catch, totally missed that.

@@ -68,8 +69,9 @@
private final Ticker ticker;
private final long startTime;
// Address -> Number of failures in a row

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

Address -> Number of consecutive failures

@@ -193,7 +212,7 @@ private synchronized void handleFailure(A address, Throwable throwable)
ExceptionClassification exceptionClassification = retryPolicy.classifyException(throwable, metadata.isIdempotent());
// update stats based on classification
attemptedAddresses.add(address);
attemptedAddresses.computeIfAbsent(address, (key) -> new AtomicInteger(0)).incrementAndGet();

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

when map it update not use atomic, use merge here

@@ -226,7 +245,19 @@ else if (exceptionClassification.getHostStatus() == OVERLOADED) {
// A request to down or overloaded server is not counted as an attempt, and retries are not delayed

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

update comment

@@ -226,7 +245,19 @@ else if (exceptionClassification.getHostStatus() == OVERLOADED) {
// A request to down or overloaded server is not counted as an attempt, and retries are not delayed
if (exceptionClassification.getHostStatus() != NORMAL) {
nextAttempt();
Optional<A> selectedAddress = addressSelector.selectAddress(addressSelectionContext, attemptedAddresses.keySet());

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

I'm not sure I like this code organization. Maybe instead of passing the address to nextAttempt, we let next attempt select the address and update stats, as it is doing now, and then check if we need to delay for that address. The obvious down side is we could double delay from request failure and connection reattempt. I think for that we could simply pass a boolean noConnectDelay to nextAttempt to signal that we have already delayed the request. I'm hoping this will keep the code fairly simple to follow.

This comment has been minimized.

@dain

dain Nov 23, 2018

Member

Also, I think we should consider logging at debug when delaying a connect attempt.

This comment has been minimized.

@arhimondr

arhimondr Nov 27, 2018

Contributor

I'm not sure I like this code organization. Maybe instead of passing the address to nextAttempt, we let next attempt select the address and update stats, as it is doing now, and then check if we need to delay for that address. The obvious down side is we could double delay from request failure and connection reattempt. I think for that we could simply pass a boolean noConnectDelay to nextAttempt to signal that we have already delayed the request. I'm hoping this will keep the code fairly simple to follow.

I reorganized the code. I added a separate map, that tracks only connection failed attempts.

@arhimondr arhimondr force-pushed the arhimondr:exponential-backoff-for-connection-retry branch 2 times, most recently from e81106d to 37b85d8 Nov 27, 2018

@dain

A couple of very minor comments.

ListenableFuture<?> delay = invoker.delay(backoffDelay);
private synchronized void schedule(Duration timeout, Runnable r)

This comment has been minimized.

@dain

dain Nov 27, 2018

Member

rename r to something more readable like task

}
@Override
public void onFailure(Throwable throwable)
public void onFailure(Throwable t)

This comment has been minimized.

@dain

dain Nov 27, 2018

Member

Why rename throwable to t?

This comment has been minimized.

@arhimondr

arhimondr Nov 27, 2018

Contributor

Just to be consistent. We use t anywhere else in this class. It is more concise.

This comment has been minimized.

@arhimondr

arhimondr Nov 27, 2018

Contributor

Sorry, actually not everywhere. We use throwable in handleFailure and unexpectedError. But we use t in all the other places where the scope of that variable is small. I can rename it back if you think it was better.

@@ -810,6 +876,48 @@ public synchronized void assertAllDown()
}
}
private static class TestingAddress

This comment has been minimized.

@dain

dain Nov 27, 2018

Member

Why not use SimpleAddress?

@arhimondr arhimondr force-pushed the arhimondr:exponential-backoff-for-connection-retry branch from 37b85d8 to 606f473 Nov 27, 2018

@@ -71,6 +72,8 @@
@GuardedBy("this")
private final Set<A> attemptedAddresses = new LinkedHashSet<>();
@GuardedBy("this")
private final Map<A, Integer> failedConnectionAttempts = new HashMap<>();

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

I think this would be cleaner as a Multiset. Then you can use the simpler count() and add() methods.

This comment has been minimized.

@dain

dain Dec 12, 2018

Member

That is even better than my suggestion.

This comment has been minimized.

@arhimondr

arhimondr Dec 12, 2018

Contributor

Thanks for the suggestion. Originally i was looking for something like this, but totally forgot how is it called in Guava (was looking for something like CountingMap).

@dain

dain approved these changes Dec 12, 2018

@@ -71,6 +72,8 @@
@GuardedBy("this")
private final Set<A> attemptedAddresses = new LinkedHashSet<>();
@GuardedBy("this")
private final Map<A, Integer> failedConnectionAttempts = new HashMap<>();

This comment has been minimized.

@dain

dain Dec 12, 2018

Member

That is even better than my suggestion.

Add delay before retrying same address
Retrying same address without a delay may lead to infinite loop

@arhimondr arhimondr force-pushed the arhimondr:exponential-backoff-for-connection-retry branch from 606f473 to 2056c1b Dec 12, 2018

@electrum electrum merged commit 800c823 into airlift:master Dec 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment