feat: optimize RedisProxyJedisPool protect logic (#459)

caojiajun · caojiajun · commit e6699ac930b8 · 2025-12-23T16:40:27.000+08:00
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/AffinityProxySelector.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/AffinityProxySelector.java
@@ -52,32 +52,51 @@ public Proxy next(Boolean affinity) {
     }
 
     @Override
-    public void ban(Proxy proxy) {
-        logger.warn("proxy {}:{} was baned", proxy.getHost(),proxy.getPort());
-        dynamicProxyQueue.remove(proxy);
+    public boolean ban(Proxy proxy) {
+        try {
+            logger.warn("proxy {}:{} was baned", proxy.getHost(),proxy.getPort());
+            dynamicProxyQueue.remove(proxy);
+            return true;
+        } catch (Exception e) {
+            logger.error("ban error, proxy = {}", proxy, e);
+            return false;
+        }
     }
 
     @Override
-    public void add(Proxy proxy) {
-        if (!dynamicProxyQueue.contains(proxy)) {
-            dynamicProxyQueue.add(proxy);
-        }
-        if (!proxyList.contains(proxy)) {
-            proxyList.add(proxy);
+    public boolean add(Proxy proxy) {
+        try {
+            if (!dynamicProxyQueue.contains(proxy)) {
+                dynamicProxyQueue.add(proxy);
+            }
+            if (!proxyList.contains(proxy)) {
+                proxyList.add(proxy);
+            }
+            return true;
+        } catch (Exception e) {
+            logger.error("add error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
     @Override
-    public void remove(Proxy proxy) {
-        if (dynamicProxyQueue.size() == 1) {
-            if (logger.isWarnEnabled()) {
-                logger.warn("proxySet.size = 1, skip remove proxy! proxy = {}", proxy);
+    public boolean remove(Proxy proxy) {
+        try {
+            if (dynamicProxyQueue.size() == 1) {
+                if (logger.isWarnEnabled()) {
+                    logger.warn("proxySet.size = 1, skip remove proxy! proxy = {}", proxy);
+                }
+            } else {
+                dynamicProxyQueue.remove(proxy);
             }
-        } else {
-            dynamicProxyQueue.remove(proxy);
-        }
-        if (proxyList.size() > 1) {
-            proxyList.remove(proxy);
+            if (proxyList.size() > 1) {
+                proxyList.remove(proxy);
+                return true;
+            }
+            return false;
+        } catch (Exception e) {
+            logger.error("remove error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/IProxySelector.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/IProxySelector.java
@@ -19,11 +19,11 @@ default Proxy next(Boolean affinity) {
         return next();
     }
 
-    void ban(Proxy proxy);
+    boolean ban(Proxy proxy);
 
-    void add(Proxy proxy);
+    boolean add(Proxy proxy);
 
-    void remove(Proxy proxy);
+    boolean remove(Proxy proxy);
 
     Set<Proxy> getAll();
 
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/RandomProxySelector.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/RandomProxySelector.java
@@ -34,38 +34,48 @@ public Proxy next() {
     }
 
     @Override
-    public void ban(Proxy proxy) {
+    public boolean ban(Proxy proxy) {
         try {
             synchronized (lock) {
                 dynamicProxyList.remove(proxy);
             }
-        } catch (Exception ignore) {
+            return true;
+        } catch (Exception e) {
+            logger.error("ban error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
     @Override
-    public void add(Proxy proxy) {
+    public boolean add(Proxy proxy) {
         try {
             synchronized (lock) {
                 proxySet.add(proxy);
                 dynamicProxyList = new ArrayList<>(proxySet);
             }
-        } catch (Exception ignore) {
+            return true;
+        } catch (Exception e) {
+            logger.error("add error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
     @Override
-    public void remove(Proxy proxy) {
+    public boolean remove(Proxy proxy) {
         try {
             synchronized (lock) {
                 if (proxySet.size() == 1) {
                     logger.warn("proxySet.size = 1, skip remove proxy! proxy = {}", proxy.toString());
+                    return false;
                 } else {
                     proxySet.remove(proxy);
                     dynamicProxyList = new ArrayList<>(proxySet);
+                    return true;
                 }
             }
-        } catch (Exception ignore) {
+        } catch (Exception e) {
+            logger.error("remove error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/SideCarFirstProxySelector.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-common/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/common/SideCarFirstProxySelector.java
@@ -76,13 +76,15 @@ private Proxy tryPick(List<Proxy> dynamicProxyList) {
     }
 
     @Override
-    public void ban(Proxy proxy) {
+    public boolean ban(Proxy proxy) {
         try {
             synchronized (lock) {
-                if (proxy == null) return;
+                if (proxy == null) {
+                    return false;
+                }
                 if (proxy.getHost().equals(localhost)) {
                     dynamicSideCarProxy = null;
-                    return;
+                    return true;
                 }
                 String region = regionResolver.resolve(proxy.getHost());
                 if (Objects.equals(region, localRegion)) {
@@ -91,20 +93,24 @@ public void ban(Proxy proxy) {
                     dynamicProxyListOtherRegion.remove(proxy);
                 }
             }
+            return true;
         } catch (Exception e) {
-            logger.error("ban error", e);
+            logger.error("ban error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
     @Override
-    public void add(Proxy proxy) {
+    public boolean add(Proxy proxy) {
         try {
             synchronized (lock) {
-                if (proxy == null) return;
+                if (proxy == null) {
+                    return false;
+                }
                 if (proxy.getHost().equals(localhost)) {
                     sideCarProxy = proxy;
                     dynamicSideCarProxy = proxy;
-                    return;
+                    return true;
                 }
                 String region = regionResolver.resolve(proxy.getHost());
                 if (Objects.equals(region, localRegion)) {
@@ -114,25 +120,27 @@ public void add(Proxy proxy) {
                     proxySetOtherRegion.add(proxy);
                     dynamicProxyListOtherRegion = new ArrayList<>(proxySetOtherRegion);
                 }
+                return true;
             }
         } catch (Exception e) {
-            logger.error("add error", e);
+            logger.error("add error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
     @Override
-    public void remove(Proxy proxy) {
+    public boolean remove(Proxy proxy) {
         try {
             synchronized (lock) {
-                if (proxy == null) return;
+                if (proxy == null) return false;
                 if (isOnlyOneProxy()) {
                     logger.warn("proxySet.size <= 1, skip remove proxy! proxy = {}", proxy);
-                    return;
+                    return false;
                 }
                 if (proxy.getHost().equals(localhost)) {
                     sideCarProxy = null;
                     dynamicSideCarProxy = null;
-                    return;
+                    return true;
                 }
                 String region = regionResolver.resolve(proxy.getHost());
                 if (Objects.equals(region, localRegion)) {
@@ -142,9 +150,11 @@ public void remove(Proxy proxy) {
                     proxySetOtherRegion.remove(proxy);
                     dynamicProxyListOtherRegion = new ArrayList<>(proxySetOtherRegion);
                 }
+                return true;
             }
         } catch (Exception e) {
-            logger.error("remove error", e);
+            logger.error("remove error, proxy = {}", proxy, e);
+            return false;
         }
     }
 
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-jedis2/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/jedis/RedisProxyJedisPool.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-jedis2/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/jedis/RedisProxyJedisPool.java
@@ -573,10 +573,12 @@ private void remove(Proxy proxy) {
         if (proxy == null) return;
         synchronized (lock) {
             try {
-                proxySelector.remove(proxy);
-                JedisPool remove = jedisPoolMap.remove(proxy);
-                if (remove != null) {
-                    remove.close();
+                boolean removed = proxySelector.remove(proxy);
+                if (removed) {
+                    JedisPool remove = jedisPoolMap.remove(proxy);
+                    if (remove != null) {
+                        remove.close();
+                    }
                 }
             } catch (Exception e) {
                 logger.error("remove proxy error, proxy = {}", proxy, e);
@@ -605,14 +607,20 @@ public void run() {
             try {
                 List<Proxy> list = proxyJedisPool.proxyDiscovery.findAll();
                 if (list != null && !list.isEmpty()) {
-                    Set<Proxy> proxySet = proxyJedisPool.proxySelector.getAll();
+                    list = new ArrayList<>(list);
+                    Collections.shuffle(list);
+                    //add
                     for (Proxy proxy : list) {
                         proxyJedisPool.add(proxy);
                     }
+                    //remove
+                    Set<Proxy> proxySet = proxyJedisPool.proxySelector.getAll();
                     Set<Proxy> oldSet = new HashSet<>(proxySet);
                     list.forEach(oldSet::remove);
                     if (!oldSet.isEmpty()) {
-                        for (Proxy proxy : oldSet) {
+                        List<Proxy> removed = new ArrayList<>(oldSet);
+                        Collections.shuffle(removed);
+                        for (Proxy proxy : removed) {
                             proxyJedisPool.remove(proxy);
                         }
                     }
diff --git a/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-jedis3/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/jedis/RedisProxyJedisPool.java b/camellia-redis-proxy/camellia-redis-proxy-extensions/camellia-redis-proxy-discovery/camellia-redis-proxy-discovery-jedis3/src/main/java/com/netease/nim/camellia/redis/proxy/discovery/jedis/RedisProxyJedisPool.java
@@ -605,14 +605,20 @@ public void run() {
             try {
                 List<Proxy> list = proxyJedisPool.proxyDiscovery.findAll();
                 if (list != null && !list.isEmpty()) {
-                    Set<Proxy> proxySet = proxyJedisPool.proxySelector.getAll();
+                    list = new ArrayList<>(new HashSet<>(list));
+                    Collections.shuffle(list);
+                    //add
                     for (Proxy proxy : list) {
                         proxyJedisPool.add(proxy);
                     }
+                    //remove
+                    Set<Proxy> proxySet = proxyJedisPool.proxySelector.getAll();
                     Set<Proxy> oldSet = new HashSet<>(proxySet);
                     list.forEach(oldSet::remove);
                     if (!oldSet.isEmpty()) {
-                        for (Proxy proxy : oldSet) {
+                        List<Proxy> removed = new ArrayList<>(oldSet);
+                        Collections.shuffle(removed);
+                        for (Proxy proxy : removed) {
                             proxyJedisPool.remove(proxy);
                         }
                     }

Original file line number	Diff line number	Diff line change
`@@ -34,38 +34,48 @@ public Proxy next() {`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`@Override`
`37`		`- public void ban(Proxy proxy) {`
	`37`	`+ public boolean ban(Proxy proxy) {`
`38`	`38`	`try {`
`39`	`39`	`synchronized (lock) {`
`40`	`40`	`dynamicProxyList.remove(proxy);`
`41`	`41`	`}`
`42`		`- } catch (Exception ignore) {`
	`42`	`+ return true;`
	`43`	`+ } catch (Exception e) {`
	`44`	`+ logger.error("ban error, proxy = {}", proxy, e);`
	`45`	`+ return false;`
`43`	`46`	`}`
`44`	`47`	`}`
`45`	`48`
`46`	`49`	`@Override`
`47`		`- public void add(Proxy proxy) {`
	`50`	`+ public boolean add(Proxy proxy) {`
`48`	`51`	`try {`
`49`	`52`	`synchronized (lock) {`
`50`	`53`	`proxySet.add(proxy);`
`51`	`54`	`dynamicProxyList = new ArrayList<>(proxySet);`
`52`	`55`	`}`
`53`		`- } catch (Exception ignore) {`
	`56`	`+ return true;`
	`57`	`+ } catch (Exception e) {`
	`58`	`+ logger.error("add error, proxy = {}", proxy, e);`
	`59`	`+ return false;`
`54`	`60`	`}`
`55`	`61`	`}`
`56`	`62`
`57`	`63`	`@Override`
`58`		`- public void remove(Proxy proxy) {`
	`64`	`+ public boolean remove(Proxy proxy) {`
`59`	`65`	`try {`
`60`	`66`	`synchronized (lock) {`
`61`	`67`	`if (proxySet.size() == 1) {`
`62`	`68`	`logger.warn("proxySet.size = 1, skip remove proxy! proxy = {}", proxy.toString());`
	`69`	`+ return false;`
`63`	`70`	`} else {`
`64`	`71`	`proxySet.remove(proxy);`
`65`	`72`	`dynamicProxyList = new ArrayList<>(proxySet);`
	`73`	`+ return true;`
`66`	`74`	`}`
`67`	`75`	`}`
`68`		`- } catch (Exception ignore) {`
	`76`	`+ } catch (Exception e) {`
	`77`	`+ logger.error("remove error, proxy = {}", proxy, e);`
	`78`	`+ return false;`
`69`	`79`	`}`
`70`	`80`	`}`
`71`	`81`
Original file line number	Diff line number	Diff line change
`@@ -76,13 +76,15 @@ private Proxy tryPick(List<Proxy> dynamicProxyList) {`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`@Override`
`79`		`- public void ban(Proxy proxy) {`
	`79`	`+ public boolean ban(Proxy proxy) {`
`80`	`80`	`try {`
`81`	`81`	`synchronized (lock) {`
`82`		`- if (proxy == null) return;`
	`82`	`+ if (proxy == null) {`
	`83`	`+ return false;`
	`84`	`+ }`
`83`	`85`	`if (proxy.getHost().equals(localhost)) {`
`84`	`86`	`dynamicSideCarProxy = null;`
`85`		`- return;`
	`87`	`+ return true;`
`86`	`88`	`}`
`87`	`89`	`String region = regionResolver.resolve(proxy.getHost());`
`88`	`90`	`if (Objects.equals(region, localRegion)) {`
`@@ -91,20 +93,24 @@ public void ban(Proxy proxy) {`
`91`	`93`	`dynamicProxyListOtherRegion.remove(proxy);`
`92`	`94`	`}`
`93`	`95`	`}`
	`96`	`+ return true;`
`94`	`97`	`} catch (Exception e) {`
`95`		`- logger.error("ban error", e);`
	`98`	`+ logger.error("ban error, proxy = {}", proxy, e);`
	`99`	`+ return false;`
`96`	`100`	`}`
`97`	`101`	`}`
`98`	`102`
`99`	`103`	`@Override`
`100`		`- public void add(Proxy proxy) {`
	`104`	`+ public boolean add(Proxy proxy) {`
`101`	`105`	`try {`
`102`	`106`	`synchronized (lock) {`
`103`		`- if (proxy == null) return;`
	`107`	`+ if (proxy == null) {`
	`108`	`+ return false;`
	`109`	`+ }`
`104`	`110`	`if (proxy.getHost().equals(localhost)) {`
`105`	`111`	`sideCarProxy = proxy;`
`106`	`112`	`dynamicSideCarProxy = proxy;`
`107`		`- return;`
	`113`	`+ return true;`
`108`	`114`	`}`
`109`	`115`	`String region = regionResolver.resolve(proxy.getHost());`
`110`	`116`	`if (Objects.equals(region, localRegion)) {`
`@@ -114,25 +120,27 @@ public void add(Proxy proxy) {`
`114`	`120`	`proxySetOtherRegion.add(proxy);`
`115`	`121`	`dynamicProxyListOtherRegion = new ArrayList<>(proxySetOtherRegion);`
`116`	`122`	`}`
	`123`	`+ return true;`
`117`	`124`	`}`
`118`	`125`	`} catch (Exception e) {`
`119`		`- logger.error("add error", e);`
	`126`	`+ logger.error("add error, proxy = {}", proxy, e);`
	`127`	`+ return false;`
`120`	`128`	`}`
`121`	`129`	`}`
`122`	`130`
`123`	`131`	`@Override`
`124`		`- public void remove(Proxy proxy) {`
	`132`	`+ public boolean remove(Proxy proxy) {`
`125`	`133`	`try {`
`126`	`134`	`synchronized (lock) {`
`127`		`- if (proxy == null) return;`
	`135`	`+ if (proxy == null) return false;`
`128`	`136`	`if (isOnlyOneProxy()) {`
`129`	`137`	`logger.warn("proxySet.size <= 1, skip remove proxy! proxy = {}", proxy);`
`130`		`- return;`
	`138`	`+ return false;`
`131`	`139`	`}`
`132`	`140`	`if (proxy.getHost().equals(localhost)) {`
`133`	`141`	`sideCarProxy = null;`
`134`	`142`	`dynamicSideCarProxy = null;`
`135`		`- return;`
	`143`	`+ return true;`
`136`	`144`	`}`
`137`	`145`	`String region = regionResolver.resolve(proxy.getHost());`
`138`	`146`	`if (Objects.equals(region, localRegion)) {`
`@@ -142,9 +150,11 @@ public void remove(Proxy proxy) {`
`142`	`150`	`proxySetOtherRegion.remove(proxy);`
`143`	`151`	`dynamicProxyListOtherRegion = new ArrayList<>(proxySetOtherRegion);`
`144`	`152`	`}`
	`153`	`+ return true;`
`145`	`154`	`}`
`146`	`155`	`} catch (Exception e) {`
`147`		`- logger.error("remove error", e);`
	`156`	`+ logger.error("remove error, proxy = {}", proxy, e);`
	`157`	`+ return false;`
`148`	`158`	`}`
`149`	`159`	`}`
`150`	`160`