Skip to content

Commit

Permalink
fix: Fix the problem of wrong number of retries in failback mode (apa…
Browse files Browse the repository at this point in the history 
…che#9525)

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 apache#9522
  • Loading branch information
@juzi214032
juzi214032 authored Jan 23, 2022
1 parent 430d439 commit e6148ac
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 e6148ac

Please sign in to comment.