liubao68 closed pull request #649: [SCB-488]Retry/Metrics some default behavior 
cause unnecessary retry and logs
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/649
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
 
b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
index 606d6bafa..f23e8822c 100644
--- 
a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
+++ 
b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
@@ -179,7 +179,7 @@ private void testFallback(RestTemplate template, String 
cseUrlPrefix) {
       result = template.getForObject(cseUrlPrefix + 
"/fallback/throwexception/throwexception", String.class);
       TestMgr.check(false, true);
     } catch (Exception e) {
-      TestMgr.check(((CseException) 
e.getCause().getCause().getCause()).getMessage(),
+      TestMgr.check(((CseException) e.getCause()).getMessage(),
           
BizkeeperExceptionUtils.createBizkeeperException(BizkeeperExceptionUtils.CSE_HANDLER_BK_FALLBACK,
               null,
               "springmvc.codeFirst.fallbackThrowException").getMessage());
diff --git 
a/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/MetricsBootstrapConfig.java
 
b/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/MetricsBootstrapConfig.java
index eabe3e3e3..d1b6be72f 100644
--- 
a/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/MetricsBootstrapConfig.java
+++ 
b/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/MetricsBootstrapConfig.java
@@ -21,7 +21,7 @@
 public class MetricsBootstrapConfig {
   public static final String METRICS_WINDOW_TIME = 
"servicecomb.metrics.window_time";
 
-  public static final int DEFAULT_METRICS_WINDOW_TIME = 5000;
+  public static final int DEFAULT_METRICS_WINDOW_TIME = 60000;
 
   private long msPollInterval;
 
diff --git 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
index ae45347b5..aad3893d1 100644
--- 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
+++ 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
@@ -59,10 +59,10 @@ protected String getCacheKey() {
 
   @Override
   protected Observable<Response> resumeWithFallback() {
-    LOG.debug("Fallback executed due to: ", getFailedExecutionException());
+    Throwable cause = getFailedExecutionException();
     Observable<Response> observable = Observable.create(f -> {
       try {
-        f.onNext(FallbackPolicyManager.getFallbackResponse(type, invocation));
+        f.onNext(FallbackPolicyManager.getFallbackResponse(type, cause, 
invocation));
         f.onCompleted();
       } catch (Exception e) {
         LOG.warn("fallback failed due to:" + e.getMessage());
diff --git 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperHandlerDelegate.java
 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperHandlerDelegate.java
index 2ef3870a5..e3b755b5d 100644
--- 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperHandlerDelegate.java
+++ 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperHandlerDelegate.java
@@ -56,7 +56,7 @@ public BizkeeperHandlerDelegate(BizkeeperHandler handler) {
       public Observable<Response> toObservable() {
         return Observable.create(f -> {
           try {
-            
f.onNext(FallbackPolicyManager.getFallbackResponse(handler.groupname, 
invocation));
+            
f.onNext(FallbackPolicyManager.getFallbackResponse(handler.groupname, null, 
invocation));
           } catch (Exception e) {
             f.onError(e);
           }
diff --git 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/FallbackPolicyManager.java
 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/FallbackPolicyManager.java
index 6f71859b2..cf0bad2c8 100644
--- 
a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/FallbackPolicyManager.java
+++ 
b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/FallbackPolicyManager.java
@@ -36,7 +36,7 @@ public static void record(String type, Invocation invocation, 
Response response,
     }
   }
 
-  public static Response getFallbackResponse(String type, Invocation 
invocation) {
+  public static Response getFallbackResponse(String type, Throwable error, 
Invocation invocation) {
     FallbackPolicy policy = getPolicy(type, invocation);
     if (policy != null) {
       return policy.getFallbackResponse(invocation);
@@ -44,7 +44,7 @@ public static Response getFallbackResponse(String type, 
Invocation invocation) {
       return Response.failResp(invocation.getInvocationType(),
           BizkeeperExceptionUtils
               
.createBizkeeperException(BizkeeperExceptionUtils.CSE_HANDLER_BK_FALLBACK,
-                  null,
+                  error,
                   
invocation.getOperationMeta().getMicroserviceQualifiedName()));
     }
   }
diff --git 
a/handlers/handler-bizkeeper/src/test/java/org/apache/servicecomb/bizkeeper/TestFallbackPolicyManager.java
 
b/handlers/handler-bizkeeper/src/test/java/org/apache/servicecomb/bizkeeper/TestFallbackPolicyManager.java
index be6f73c7f..04672eac1 100644
--- 
a/handlers/handler-bizkeeper/src/test/java/org/apache/servicecomb/bizkeeper/TestFallbackPolicyManager.java
+++ 
b/handlers/handler-bizkeeper/src/test/java/org/apache/servicecomb/bizkeeper/TestFallbackPolicyManager.java
@@ -47,7 +47,7 @@ public void testFallbackPolicyManager(final @Mocked 
Configuration config, final
       }
     };
 
-    Assert.assertEquals((String) null, 
FallbackPolicyManager.getFallbackResponse("Consumer", invocation).getResult());
+    Assert.assertEquals((String) null, 
FallbackPolicyManager.getFallbackResponse("Consumer", null, 
invocation).getResult());
 
     new Expectations() {
       {
@@ -62,7 +62,7 @@ public void testFallbackPolicyManager(final @Mocked 
Configuration config, final
       }
     };
     Assert.assertEquals(CseException.class,
-        ((Exception) FallbackPolicyManager.getFallbackResponse("Consumer", 
invocation).getResult()).getCause()
+        ((Exception) FallbackPolicyManager.getFallbackResponse("Consumer", 
null, invocation).getResult()).getCause()
             .getClass());
 
     new Expectations() {
@@ -82,7 +82,7 @@ public void testFallbackPolicyManager(final @Mocked 
Configuration config, final
     FallbackPolicyManager.record("Consumer", invocation, 
Response.succResp("mockedsuccess"), true);
     FallbackPolicyManager.record("Consumer", invocation, 
Response.succResp("mockedfailure"), false);
     Assert.assertEquals("mockedsuccess",
-        FallbackPolicyManager.getFallbackResponse("Consumer", 
invocation).getResult());
+        FallbackPolicyManager.getFallbackResponse("Consumer", null, 
invocation).getResult());
 
     new Expectations() {
       {
@@ -97,7 +97,7 @@ public void testFallbackPolicyManager(final @Mocked 
Configuration config, final
       }
     };
     Assert.assertEquals(CseException.class,
-        ((Exception) FallbackPolicyManager.getFallbackResponse("Consumer", 
invocation).getResult()).getCause()
+        ((Exception) FallbackPolicyManager.getFallbackResponse("Consumer", 
null, invocation).getResult()).getCause()
             .getClass());
   }
 }
diff --git 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/DefaultRetryExtensionsFactory.java
 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/DefaultRetryExtensionsFactory.java
index 138785b5b..546c3e33e 100644
--- 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/DefaultRetryExtensionsFactory.java
+++ 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/DefaultRetryExtensionsFactory.java
@@ -23,6 +23,7 @@
 import com.google.common.collect.Lists;
 import com.netflix.client.DefaultLoadBalancerRetryHandler;
 import com.netflix.client.RetryHandler;
+import com.netflix.client.Utils;
 
 @Component
 public class DefaultRetryExtensionsFactory implements ExtensionsFactory {
@@ -42,7 +43,12 @@ public boolean isSupport(String key, String value) {
   public RetryHandler createRetryHandler(String retryName, String 
microservice) {
     RetryHandler handler = new DefaultLoadBalancerRetryHandler(
         Configuration.INSTANCE.getRetryOnSame(microservice),
-        Configuration.INSTANCE.getRetryOnNext(microservice), true);
+        Configuration.INSTANCE.getRetryOnNext(microservice), true) {
+      @Override
+      public boolean isRetriableException(Throwable e, boolean sameServer) {
+        return Utils.isPresentAsCause(e, getRetriableExceptions());
+      }
+    };
     return handler;
   }
 }
diff --git 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
index ec656918f..880da701b 100644
--- 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
+++ 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
@@ -37,7 +37,7 @@
 public class LoadBalancer extends AbstractLoadBalancer {
   private String name;
 
-  private List<Server> serverList;
+  private List<Server> serverList = Collections.emptyList();
 
   private IRule rule;
 
diff --git 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
index fd22f8ec5..7cff44e76 100644
--- 
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
+++ 
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
@@ -41,6 +41,8 @@
 import org.apache.servicecomb.serviceregistry.discovery.DiscoveryTree;
 import org.apache.servicecomb.swagger.invocation.AsyncResponse;
 import org.apache.servicecomb.swagger.invocation.Response;
+import org.apache.servicecomb.swagger.invocation.exception.ExceptionFactory;
+import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -260,7 +262,7 @@ public void onExecutionFailed(ExecutionContext<Invocation> 
context, Throwable fi
             invocation.setHandlerIndex(currentHandler); // for retry
             invocation.setEndpoint(((CseServer) s).getEndpoint());
             invocation.next(resp -> {
-              if (resp.isFailed()) {
+              if (isFailedResponse(resp)) {
                 LOGGER.error("service call error, msg is {}, server is {} ",
                     ((Throwable) resp.getResult()).getMessage(),
                     s);
@@ -290,6 +292,19 @@ public void onExecutionFailed(ExecutionContext<Invocation> 
context, Throwable fi
     });
   }
 
+  protected boolean isFailedResponse(Response resp) {
+    if (resp.isFailed()) {
+      if (InvocationException.class.isInstance(resp.getResult())) {
+        InvocationException e = (InvocationException) resp.getResult();
+        return e.getStatusCode() == 
ExceptionFactory.CONSUMER_INNER_STATUS_CODE;
+      } else {
+        return true;
+      }
+    } else {
+      return false;
+    }
+  }
+
   protected LoadBalancer getOrCreateLoadBalancer(Invocation invocation) {
     DiscoveryContext context = new DiscoveryContext();
     context.setInputParameters(invocation);
diff --git 
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestExtensionsManager.java
 
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestExtensionsManager.java
index 5232c3911..efed7d743 100644
--- 
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestExtensionsManager.java
+++ 
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestExtensionsManager.java
@@ -16,17 +16,21 @@
  */
 package org.apache.servicecomb.loadbalance;
 
+import java.net.ConnectException;
+import java.net.SocketTimeoutException;
 import java.util.ArrayList;
 import java.util.List;
 
 import org.apache.servicecomb.config.ConfigUtil;
 import org.apache.servicecomb.foundation.test.scaffolding.config.ArchaiusUtils;
+import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
 import com.netflix.client.DefaultLoadBalancerRetryHandler;
+import com.netflix.client.RetryHandler;
 import com.netflix.loadbalancer.RandomRule;
 import com.netflix.loadbalancer.RoundRobinRule;
 import com.netflix.loadbalancer.WeightedResponseTimeRule;
@@ -109,7 +113,13 @@ public void testRuleClassName() {
     
System.getProperties().remove("cse.loadbalance.mytest3.NFLoadBalancerRuleClassName");
     
System.getProperties().remove("cse.loadbalance.mytest4.NFLoadBalancerRuleClassName");
 
-    Assert.assertEquals(DefaultLoadBalancerRetryHandler.class.getName(),
-        ExtensionsManager.createRetryHandler("mytest1").getClass().getName());
+    RetryHandler retryHandler = 
ExtensionsManager.createRetryHandler("mytest1");
+    
Assert.assertTrue(DefaultLoadBalancerRetryHandler.class.isInstance(retryHandler));
+    Assert.assertFalse(retryHandler.isRetriableException(new 
InvocationException(400, "", ""), false));
+    Assert.assertFalse(retryHandler.isRetriableException(new 
InvocationException(400, "", ""), true));
+    Assert.assertTrue(retryHandler.isRetriableException(new 
ConnectException(), false));
+    Assert.assertTrue(retryHandler.isRetriableException(new 
ConnectException(), true));
+    Assert.assertTrue(retryHandler.isRetriableException(new 
SocketTimeoutException(), false));
+    Assert.assertTrue(retryHandler.isRetriableException(new 
SocketTimeoutException(), true));
   }
 }
diff --git 
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadbalanceHandler.java
 
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadbalanceHandler.java
index 85c91a796..b8138a39a 100644
--- 
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadbalanceHandler.java
+++ 
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadbalanceHandler.java
@@ -450,4 +450,12 @@ public void testIsEqual() {
     boolean localNotNull = 
handler.isEqual("com.netflix.loadbalancer.RandomRule", null);
     Assert.assertEquals(false, localNotNull);
   }
+
+  @Test
+  public void testIsFailedResponse() {
+    Assert.assertFalse(handler.isFailedResponse(Response.create(400, "", "")));
+    Assert.assertFalse(handler.isFailedResponse(Response.create(500, "", "")));
+    Assert.assertTrue(handler.isFailedResponse(Response.create(490, "", "")));
+    Assert.assertTrue(handler.isFailedResponse(Response.consumerFailResp(new 
NullPointerException())));
+  }
 }
diff --git 
a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java
 
b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java
index a4efedd15..64861c735 100644
--- 
a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java
+++ 
b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java
@@ -50,7 +50,7 @@
   @Override
   public void init(CompositeRegistry globalRegistry, EventBus eventBus, 
MetricsBootstrapConfig config) {
     if (!DynamicPropertyFactory.getInstance()
-        .getBooleanProperty(ENABLED, true)
+        .getBooleanProperty(ENABLED, false)
         .get()) {
       return;
     }
diff --git 
a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/publish/TestDefaultLogPublisher.java
 
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/publish/TestDefaultLogPublisher.java
index c8ffc5550..57b107415 100644
--- 
a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/publish/TestDefaultLogPublisher.java
+++ 
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/publish/TestDefaultLogPublisher.java
@@ -73,7 +73,7 @@ public void teardown() {
 
   @Test
   public void init_enabled_default() {
-    Holder<Boolean> registered = new Holder<>();
+    Holder<Boolean> registered = new Holder<>(false);
     new MockUp<EventBus>(eventBus) {
       @Mock
       void register(Object object) {
@@ -82,7 +82,7 @@ void register(Object object) {
     };
 
     publisher.init(globalRegistry, eventBus, new MetricsBootstrapConfig());
-    Assert.assertTrue(registered.value);
+    Assert.assertFalse(registered.value);
   }
 
   @Test


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to