Skip to content

Commit

Permalink
fix: Fix the problem of wrong number of retries in failback mode
Browse files Browse the repository at this point in the history 
When the retries value is zero,
the logic in failback mode resets it to the default value of 3,
making it impossible to turn off the retry mechanism.

Fixes #9522
  • Loading branch information
@juzi214032
juzi214032 committed Jan 4, 2022
1 parent 0d9b094 commit 5568c98
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 4 deletions.
21 changes: 17 additions & 4 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/support/FailbackClusterInvoker.java
View file
Expand Up @@ -52,6 +52,9 @@ public class FailbackClusterInvoker<T> extends AbstractClusterInvoker<T> {

private static final long RETRY_FAILED_PERIOD = 5;

/**
* Number of retries obtained from the configuration, don't contain the first invoke.
*/
private final int retries;

private final int failbackTasks;
Expand All @@ -62,7 +65,7 @@ public FailbackClusterInvoker(Directory<T> directory) {
super(directory);

int retriesConfig = getUrl().getParameter(RETRIES_KEY, DEFAULT_FAILBACK_TIMES);
if (retriesConfig <= 0) {
if (retriesConfig < 0) {
retriesConfig = DEFAULT_FAILBACK_TIMES;
}
int failbackTasksConfig = getUrl().getParameter(FAIL_BACK_TASKS_KEY, DEFAULT_FAILBACK_TASKS);
Expand Down Expand Up @@ -124,10 +127,18 @@ private class RetryTimerTask implements TimerTask {
private final Invocation invocation;
private final LoadBalance loadbalance;
private final List<Invoker<T>> invokers;
private final int retries;
private final long tick;
private Invoker<T> lastInvoker;
private int retryTimes = 0;

/**
* Number of retries obtained from the configuration, don't contain the first invoke.
*/
private final int retries;

/**
* Number of retried.
*/
private int retriedTimes = 0;

RetryTimerTask(LoadBalance loadbalance, Invocation invocation, List<Invoker<T>> invokers, Invoker<T> lastInvoker, int retries, long tick) {
this.loadbalance = loadbalance;
Expand All @@ -141,12 +152,14 @@ private class RetryTimerTask implements TimerTask {
@Override
public void run(Timeout timeout) {
try {
logger.info("Attempt to retry to invoke method " + invocation.getMethodName() +
". The total will retry " + retries + " times, the current is the " + retriedTimes + " retry");
Invoker<T> retryInvoker = select(loadbalance, invocation, invokers, Collections.singletonList(lastInvoker));
lastInvoker = retryInvoker;
retryInvoker.invoke(invocation);
} catch (Throwable e) {
logger.error("Failed retry to invoke method " + invocation.getMethodName() + ", waiting again.", e);
if ((++retryTimes) >= retries) {
if ((++retriedTimes) >= retries) {
logger.error("Failed retry times exceed threshold (" + retries + "), We have to abandon, invocation->" + invocation);
} else {
rePut(timeout);
Expand Down
96 changes: 96 additions & 0 deletions ...luster/src/test/java/org/apache/dubbo/rpc/cluster/support/FailbackClusterInvokerTest.java
View file
Expand Up @@ -18,6 +18,7 @@
package org.apache.dubbo.rpc.cluster.support;

import org.apache.dubbo.common.URL;
import org.apache.dubbo.common.constants.CommonConstants;
import org.apache.dubbo.common.utils.DubboAppender;
import org.apache.dubbo.common.utils.LogUtil;
import org.apache.dubbo.rpc.AppResponse;
Expand All @@ -37,11 +38,13 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestMethodOrder;

import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

import static org.apache.dubbo.common.constants.CommonConstants.RETRIES_KEY;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -174,4 +177,97 @@ public void testARetryFailed() throws Exception {
Assertions.assertEquals(1, LogUtil.findMessage(Level.ERROR, "Failback background works error"), "must have one error message ");
// it can be invoke successfully
}


private long getRetryFailedPeriod() throws NoSuchFieldException, IllegalAccessException {
Field retryFailedPeriod = FailbackClusterInvoker.class.getDeclaredField("RETRY_FAILED_PERIOD");
retryFailedPeriod.setAccessible(true);
return retryFailedPeriod.getLong(FailbackClusterInvoker.class);
}

@Test
@Order(5)
public void testInvokeRetryTimesWithZeroValue() throws InterruptedException, NoSuchFieldException,
IllegalAccessException {
int retries = 0;
resetInvokerToException();
given(dic.getConsumerUrl()).willReturn(url.addParameter(RETRIES_KEY, retries));

FailbackClusterInvoker<FailbackClusterInvokerTest> invoker = new FailbackClusterInvoker<>(dic);
LogUtil.start();
DubboAppender.clear();

invocation.setMethodName("testInvokeRetryTimesWithZeroValue");
invoker.invoke(invocation);

CountDownLatch countDown = new CountDownLatch(1);
countDown.await(getRetryFailedPeriod() * (retries + 1), TimeUnit.SECONDS);
LogUtil.stop();
Assertions.assertEquals(0, LogUtil.findMessage(Level.INFO, "Attempt to retry to invoke method " +
"testInvokeRetryTimesWithZeroValue"), "No retry messages allowed");
}

@Test
@Order(6)
public void testInvokeRetryTimesWithTwoValue() throws InterruptedException, NoSuchFieldException,
IllegalAccessException {
int retries = 2;
resetInvokerToException();
given(dic.getConsumerUrl()).willReturn(url.addParameter(RETRIES_KEY, retries));

FailbackClusterInvoker<FailbackClusterInvokerTest> invoker = new FailbackClusterInvoker<>(dic);
LogUtil.start();
DubboAppender.clear();

invocation.setMethodName("testInvokeRetryTimesWithTwoValue");
invoker.invoke(invocation);

CountDownLatch countDown = new CountDownLatch(1);
countDown.await(getRetryFailedPeriod() * (retries + 1), TimeUnit.SECONDS);
LogUtil.stop();
Assertions.assertEquals(2, LogUtil.findMessage(Level.INFO, "Attempt to retry to invoke method " +
"testInvokeRetryTimesWithTwoValue"), "Must have two error message ");
}

@Test
@Order(7)
public void testInvokeRetryTimesWithDefaultValue() throws InterruptedException, NoSuchFieldException,
IllegalAccessException {
resetInvokerToException();
given(dic.getConsumerUrl()).willReturn(URL.valueOf("test://test:11/test"));

FailbackClusterInvoker<FailbackClusterInvokerTest> invoker = new FailbackClusterInvoker<>(dic);
LogUtil.start();
DubboAppender.clear();

invocation.setMethodName("testInvokeRetryTimesWithDefaultValue");
invoker.invoke(invocation);

CountDownLatch countDown = new CountDownLatch(1);
countDown.await(getRetryFailedPeriod() * (CommonConstants.DEFAULT_FAILBACK_TIMES + 1), TimeUnit.SECONDS);
LogUtil.stop();
Assertions.assertEquals(3, LogUtil.findMessage(Level.INFO, "Attempt to retry to invoke method " +
"testInvokeRetryTimesWithDefaultValue"), "Must have three error message ");
}

@Test
@Order(8)
public void testInvokeRetryTimesWithIllegalValue() throws InterruptedException, NoSuchFieldException,
IllegalAccessException {
resetInvokerToException();
given(dic.getConsumerUrl()).willReturn(url.addParameter(RETRIES_KEY, -100));

FailbackClusterInvoker<FailbackClusterInvokerTest> invoker = new FailbackClusterInvoker<>(dic);
LogUtil.start();
DubboAppender.clear();

invocation.setMethodName("testInvokeRetryTimesWithIllegalValue");
invoker.invoke(invocation);

CountDownLatch countDown = new CountDownLatch(1);
countDown.await(getRetryFailedPeriod() * (CommonConstants.DEFAULT_FAILBACK_TIMES + 1), TimeUnit.SECONDS);
LogUtil.stop();
Assertions.assertEquals(3, LogUtil.findMessage(Level.INFO, "Attempt to retry to invoke method " +
"testInvokeRetryTimesWithIllegalValue"), "Must have three error message ");
}
}

0 comments on commit 5568c98

Please sign in to comment.