This is an automated email from the ASF dual-hosted git repository.

liubao pushed a commit to branch master
in repository 
https://gitbox.apache.org/repos/asf/incubator-servicecomb-java-chassis.git


The following commit(s) were added to refs/heads/master by this push:
     new 41049be  [SCB-488]Retry/Metrics some default behavior cause 
unnecessary retry and logs
41049be is described below

commit 41049be9d9b41d32fbdf4e12f613d6e85ab21301
Author: liubao <bao....@huawei.com>
AuthorDate: Fri Apr 13 17:51:26 2018 +0800

    [SCB-488]Retry/Metrics some default behavior cause unnecessary retry and 
logs
---
 .../client/CodeFirstRestTemplateSpringmvc.java          |  2 +-
 .../foundation/metrics/MetricsBootstrapConfig.java      |  2 +-
 .../apache/servicecomb/bizkeeper/BizkeeperCommand.java  |  4 ++--
 .../servicecomb/bizkeeper/BizkeeperHandlerDelegate.java |  2 +-
 .../servicecomb/bizkeeper/FallbackPolicyManager.java    |  4 ++--
 .../bizkeeper/TestFallbackPolicyManager.java            |  8 ++++----
 .../loadbalance/DefaultRetryExtensionsFactory.java      |  8 +++++++-
 .../apache/servicecomb/loadbalance/LoadBalancer.java    |  2 +-
 .../servicecomb/loadbalance/LoadbalanceHandler.java     | 17 ++++++++++++++++-
 .../servicecomb/loadbalance/TestExtensionsManager.java  | 14 ++++++++++++--
 .../servicecomb/loadbalance/TestLoadbalanceHandler.java |  8 ++++++++
 .../metrics/core/publish/DefaultLogPublisher.java       |  2 +-
 .../metrics/core/publish/TestDefaultLogPublisher.java   |  4 ++--
 13 files changed, 58 insertions(+), 19 deletions(-)

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 606d6ba..f23e882 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 @@ public class CodeFirstRestTemplateSpringmvc extends 
CodeFirstRestTemplate {
       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 eabe3e3..d1b6be7 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 @@ import com.netflix.config.DynamicPropertyFactory;
 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 ae45347..aad3893 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 @@ public abstract class BizkeeperCommand extends 
HystrixObservableCommand<Response
 
   @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 2ef3870..e3b755b 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 class BizkeeperHandlerDelegate {
       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 6f71859..cf0bad2 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 class FallbackPolicyManager {
     }
   }
 
-  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 class FallbackPolicyManager {
       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 be6f73c..04672ea 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 class TestFallbackPolicyManager {
       }
     };
 
-    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 class TestFallbackPolicyManager {
       }
     };
     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 class TestFallbackPolicyManager {
     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 class TestFallbackPolicyManager {
       }
     };
     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 138785b..546c3e3 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 org.springframework.stereotype.Component;
 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 class DefaultRetryExtensionsFactory implements 
ExtensionsFactory {
   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 ec65691..880da70 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 @@ import com.netflix.loadbalancer.ServerListFilter;
 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 fd22f8e..7cff44e 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.DiscoveryFilter;
 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 class LoadbalanceHandler implements Handler {
             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 class LoadbalanceHandler implements Handler {
     });
   }
 
+  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 5232c39..efed7d7 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 class TestExtensionsManager {
     
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 85c91a7..b8138a3 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 class TestLoadbalanceHandler {
     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 a4efedd..64861c7 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 @@ public class DefaultLogPublisher implements 
MetricsInitializer {
   @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 c8ffc55..57b1074 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 class TestDefaultLogPublisher {
 
   @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 @@ public class TestDefaultLogPublisher {
     };
 
     publisher.init(globalRegistry, eventBus, new MetricsBootstrapConfig());
-    Assert.assertTrue(registered.value);
+    Assert.assertFalse(registered.value);
   }
 
   @Test

-- 
To stop receiving notification emails like this one, please contact
liu...@apache.org.

Reply via email to