optimize camellia-feign fallback logic

netease-im · Nov 7, 2023 · 7d1e918 · 7d1e918
1 parent 9634347
commit 7d1e918
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 21 deletions.
diff --git a/camellia-core/src/main/java/com/netease/nim/camellia/core/discovery/GlobalDiscoveryEnv.java b/camellia-core/src/main/java/com/netease/nim/camellia/core/discovery/GlobalDiscoveryEnv.java
@@ -0,0 +1,9 @@
+package com.netease.nim.camellia.core.discovery;
+
+/**
+ * Created by caojiajun on 2023/11/7
+ */
+public class GlobalDiscoveryEnv {
+
+    public static boolean logInfoEnable = false;
+}
diff --git a/...ore/src/main/java/com/netease/nim/camellia/core/discovery/HashCamelliaServerSelector.java b/...ore/src/main/java/com/netease/nim/camellia/core/discovery/HashCamelliaServerSelector.java
@@ -1,16 +1,26 @@
 package com.netease.nim.camellia.core.discovery;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.List;
 import java.util.concurrent.ThreadLocalRandom;
 
 public class HashCamelliaServerSelector<T> implements CamelliaServerSelector<T> {
 
+    private static final Logger logger = LoggerFactory.getLogger(HashCamelliaServerSelector.class);
+
     @Override
     public T pick(List<T> list, Object loadBalanceKey) {
         try {
-            if (list == null || list.isEmpty()) return null;
+            if (list == null || list.isEmpty()) {
+                return null;
+            }
             int size = list.size();
             if (size == 1) {
+                if (GlobalDiscoveryEnv.logInfoEnable) {
+                    logger.info("pick server by hash, only one, loadBalanceKey = {}, server = {}", loadBalanceKey, list);
+                }
                 return list.get(0);
             }
             int index;
@@ -19,6 +29,9 @@ public T pick(List<T> list, Object loadBalanceKey) {
             } else {
                 index = Math.abs(loadBalanceKey.hashCode()) % list.size();
             }
+            if (GlobalDiscoveryEnv.logInfoEnable) {
+                logger.info("pick server by hash, loadBalanceKey = {}, index = {}, list = {}", loadBalanceKey, index, list);
+            }
             return list.get(index);
         } catch (Exception e) {
             return null;

diff --git a/...ign-client/camellia-feign/src/main/java/com/netease/nim/camellia/feign/FeignCallback.java b/...ign-client/camellia-feign/src/main/java/com/netease/nim/camellia/feign/FeignCallback.java
@@ -176,7 +176,7 @@ private Object invoke(Resource resource, Object loadBalanceKey, Method method, O
         } catch (Throwable e) {
             Throwable error = ExceptionUtils.onError(e);
             success = feignEnv.getFallbackExceptionChecker().isSkipError(error);
-            if (!(error instanceof CamelliaCircuitBreakerException)) {
+            if (!success && !(error instanceof CamelliaCircuitBreakerException)) {
                 pool.onError(feignResource);
             }
             if (!success && failureListener != null) {

diff --git a/...a-feign/src/main/java/com/netease/nim/camellia/feign/discovery/DiscoveryResourcePool.java b/...a-feign/src/main/java/com/netease/nim/camellia/feign/discovery/DiscoveryResourcePool.java
@@ -3,16 +3,15 @@
 import com.netease.nim.camellia.core.discovery.CamelliaDiscovery;
 import com.netease.nim.camellia.core.discovery.CamelliaServerSelector;
 import com.netease.nim.camellia.core.discovery.CamelliaServerHealthChecker;
+import com.netease.nim.camellia.core.discovery.GlobalDiscoveryEnv;
 import com.netease.nim.camellia.feign.GlobalCamelliaFeignEnv;
 import com.netease.nim.camellia.feign.resource.FeignDiscoveryResource;
 import com.netease.nim.camellia.feign.resource.FeignResource;
 import com.netease.nim.camellia.tools.cache.CamelliaLocalCache;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
+import java.util.*;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 
@@ -104,13 +103,19 @@ public FeignResource getResource(Object loadBalanceKey) {
     public void onError(FeignResource feignResource) {
         try {
             synchronized (lock) {
-                dynamicList.remove(feignResource);
+                Set<FeignResource> set = new HashSet<>(dynamicList);
+                set.remove(feignResource);
+                List<FeignResource> list = new ArrayList<>(set);
+                Collections.sort(list);
+                dynamicList = new ArrayList<>(list);
                 if (dynamicList.isEmpty()) {
                     GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(originalList));
                 } else {
-                    Collections.sort(dynamicList);
                     GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(dynamicList));
                 }
+                if (GlobalDiscoveryEnv.logInfoEnable) {
+                    logger.info("onError feignResource = {}, dynamicList = {}, originalList = {}", feignResource, dynamicList, originalList);
+                }
             }
         } catch (Exception e) {
             logger.error("onError error", e);
@@ -120,10 +125,16 @@ public void onError(FeignResource feignResource) {
     private void add(FeignResource feignResource) {
         try {
             synchronized (lock) {
-                originalList.add(feignResource);
-                Collections.sort(originalList);
-                dynamicList = new ArrayList<>(originalList);
-                GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(dynamicList));
+                Set<FeignResource> set = new HashSet<>(originalList);
+                set.add(feignResource);
+                List<FeignResource> list = new ArrayList<>(set);
+                Collections.sort(list);
+                originalList = new ArrayList<>(list);
+                dynamicList = new ArrayList<>(list);
+                GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(list));
+                if (GlobalDiscoveryEnv.logInfoEnable) {
+                    logger.info("add feignResource = {}, dynamicList = {}, originalList = {}", feignResource, dynamicList, originalList);
+                }
             }
         } catch (Exception e) {
             logger.error("add error", e);
@@ -133,16 +144,20 @@ private void add(FeignResource feignResource) {
     private void remove(FeignResource feignResource) {
         try {
             synchronized (lock) {
-                ArrayList<FeignResource> list = new ArrayList<>(originalList);
-                list.remove(feignResource);
-                if (list.isEmpty()) {
+                Set<FeignResource> set = new HashSet<>(originalList);
+                set.remove(feignResource);
+                if (set.isEmpty()) {
                     logger.warn("last server, skip remove");
                     return;
                 }
+                List<FeignResource> list = new ArrayList<>(set);
                 Collections.sort(list);
-                originalList = list;
-                dynamicList = new ArrayList<>(originalList);
-                GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(dynamicList));
+                originalList = new ArrayList<>(list);
+                dynamicList = new ArrayList<>(list);
+                GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(list));
+                if (GlobalDiscoveryEnv.logInfoEnable) {
+                    logger.info("remove feignResource = {}, dynamicList = {}, originalList = {}", feignResource, dynamicList, originalList);
+                }
             }
         } catch (Exception e) {
             logger.error("remove error", e);
@@ -170,10 +185,14 @@ private void reload() {
         for (FeignServerInfo feignServerInfo : all) {
             list.add(toFeignResource(feignServerInfo));
         }
+        Collections.sort(list);
         this.originalList = new ArrayList<>(list);
-        this.dynamicList = new ArrayList<>(originalList);
+        this.dynamicList = new ArrayList<>(list);
 
-        GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(dynamicList));
+        GlobalCamelliaFeignEnv.register(discoveryResource, serverSelector, new ArrayList<>(list));
+        if (GlobalDiscoveryEnv.logInfoEnable) {
+            logger.info("reload, dynamicList = {}, originalList = {}", dynamicList, originalList);
+        }
     }
 
     private FeignResource toFeignResource(FeignServerInfo feignServerInfo) {

diff --git a/...amellia-feign/src/main/java/com/netease/nim/camellia/feign/naked/CamelliaNakedClient.java b/...amellia-feign/src/main/java/com/netease/nim/camellia/feign/naked/CamelliaNakedClient.java
@@ -440,11 +440,11 @@ private W invoke(OperationType operationType, Resource resource, R request, int
                 } catch (CamelliaNakedClientRetriableException e) {
                     retry ++;
                     throwException = e;
-                    if (feignResource != null) {
+                    if (feignResource != null && !feignEnv.getFallbackExceptionChecker().isSkipError(e)) {
                         resourcePool.onError(feignResource);
                     }
                 } catch (Exception e) {
-                    if (feignResource != null) {
+                    if (feignResource != null && !feignEnv.getFallbackExceptionChecker().isSkipError(e)) {
                         resourcePool.onError(feignResource);
                     }
                     throw e;