Replace remaining FPs in ConnectionPoolConfigImpl with niws style (#1784

)

* Switch ConnectionPoolConfigImpl to only use niws configuration

* Add unit tests

* Import order

zuul-core/src/main/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImpl.java

@@ -19,8 +19,6 @@
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.client.config.IClientConfigKey;
-import com.netflix.config.CachedDynamicBooleanProperty;
-import com.netflix.config.CachedDynamicIntProperty;
 import com.netflix.zuul.origins.OriginName;
 import java.util.Objects;
 
@@ -29,49 +27,47 @@
  */
 public class ConnectionPoolConfigImpl implements ConnectionPoolConfig {
 
-    private static final int DEFAULT_BUFFER_SIZE = 32 * 1024;
-    private static final int DEFAULT_CONNECT_TIMEOUT = 500;
-    private static final int DEFAULT_IDLE_TIMEOUT = 60000;
-    private static final int DEFAULT_MAX_CONNS_PER_HOST = 50;
-    private static final int DEFAULT_PER_SERVER_WATERLINE = 4;
+    static final int DEFAULT_BUFFER_SIZE = 32 * 1024;
+    static final int DEFAULT_CONNECT_TIMEOUT = 500;
+    static final int DEFAULT_IDLE_TIMEOUT = 60000;
+    static final int DEFAULT_MAX_CONNS_PER_HOST = 50;
+    static final int DEFAULT_PER_SERVER_WATERLINE = 4;
+    static final int DEFAULT_MAX_REQUESTS_PER_CONNECTION = 1000;
+
+    // TODO(argha-c): Document why these values were chosen, as opposed to defaults of 32k/64k
+    static final int DEFAULT_WRITE_BUFFER_HIGH_WATER_MARK = 32 * 1024;
+    static final int DEFAULT_WRITE_BUFFER_LOW_WATER_MARK = 8 * 1024;
 
     /**
      * NOTE that each eventloop has its own connection pool per host, and this is applied per event-loop.
      */
     public static final IClientConfigKey<Integer> PER_SERVER_WATERLINE =
-            new CommonClientConfigKey<>("PerServerWaterline", DEFAULT_PER_SERVER_WATERLINE) {};
+            new CommonClientConfigKey<>("PerServerWaterline") {};
 
     public static final IClientConfigKey<Boolean> CLOSE_ON_CIRCUIT_BREAKER =
-            new CommonClientConfigKey<>("CloseOnCircuitBreaker", true) {};
+            new CommonClientConfigKey<>("CloseOnCircuitBreaker") {};
 
-    private final OriginName originName;
-    private final IClientConfig clientConfig;
+    public static final IClientConfigKey<Integer> MAX_REQUESTS_PER_CONNECTION =
+            new CommonClientConfigKey<>("maxRequestsPerConnection") {};
 
-    private final CachedDynamicIntProperty MAX_REQUESTS_PER_CONNECTION;
+    public static final IClientConfigKey<Boolean> TCP_KEEP_ALIVE = new CommonClientConfigKey<>("TcpKeepAlive") {};
 
-    private final CachedDynamicBooleanProperty SOCKET_KEEP_ALIVE;
-    private final CachedDynamicBooleanProperty TCP_NO_DELAY;
-    private final CachedDynamicIntProperty WRITE_BUFFER_HIGH_WATER_MARK;
-    private final CachedDynamicIntProperty WRITE_BUFFER_LOW_WATER_MARK;
-    private final CachedDynamicBooleanProperty AUTO_READ;
+    public static final IClientConfigKey<Boolean> TCP_NO_DELAY = new CommonClientConfigKey<>("TcpKeepAlive") {};
 
-    public ConnectionPoolConfigImpl(final OriginName originName, IClientConfig clientConfig) {
-        this.originName = Objects.requireNonNull(originName, "originName");
-        String niwsClientName = originName.getNiwsClientName();
-        this.clientConfig = clientConfig;
+    public static final IClientConfigKey<Boolean> AUTO_READ = new CommonClientConfigKey<>("AutoRead") {};
 
-        this.MAX_REQUESTS_PER_CONNECTION =
-                new CachedDynamicIntProperty(niwsClientName + ".netty.client.maxRequestsPerConnection", 1000);
+    public static final IClientConfigKey<Integer> WRITE_BUFFER_HIGH_WATER_MARK =
+            new CommonClientConfigKey<>("WriteBufferHighWaterMark") {};
 
-        this.SOCKET_KEEP_ALIVE = new CachedDynamicBooleanProperty(niwsClientName + ".netty.client.TcpKeepAlive", false);
-        this.TCP_NO_DELAY = new CachedDynamicBooleanProperty(niwsClientName + ".netty.client.TcpNoDelay", false);
+    public static final IClientConfigKey<Integer> WRITE_BUFFER_LOW_WATER_MARK =
+            new CommonClientConfigKey<>("WriteBufferLowWaterMark") {};
 
-        // TODO(argha-c): Document why these values were chosen, as opposed to defaults of 32k/64k
-        this.WRITE_BUFFER_HIGH_WATER_MARK =
-                new CachedDynamicIntProperty(niwsClientName + ".netty.client.WriteBufferHighWaterMark", 32 * 1024);
-        this.WRITE_BUFFER_LOW_WATER_MARK =
-                new CachedDynamicIntProperty(niwsClientName + ".netty.client.WriteBufferLowWaterMark", 8 * 1024);
-        this.AUTO_READ = new CachedDynamicBooleanProperty(niwsClientName + ".netty.client.AutoRead", false);
+    private final OriginName originName;
+    private final IClientConfig clientConfig;
+
+    public ConnectionPoolConfigImpl(final OriginName originName, IClientConfig clientConfig) {
+        this.originName = Objects.requireNonNull(originName, "originName");
+        this.clientConfig = clientConfig;
     }
 
     @Override
@@ -86,7 +82,7 @@ public int getConnectTimeout() {
 
     @Override
     public int getMaxRequestsPerConnection() {
-        return MAX_REQUESTS_PER_CONNECTION.get();
+        return clientConfig.getPropertyAsInteger(MAX_REQUESTS_PER_CONNECTION, DEFAULT_MAX_REQUESTS_PER_CONNECTION);
     }
 
     @Override
@@ -108,12 +104,12 @@ public int getIdleTimeout() {
 
     @Override
     public boolean getTcpKeepAlive() {
-        return SOCKET_KEEP_ALIVE.get();
+        return clientConfig.getPropertyAsBoolean(TCP_KEEP_ALIVE, false);
     }
 
     @Override
     public boolean getTcpNoDelay() {
-        return TCP_NO_DELAY.get();
+        return clientConfig.getPropertyAsBoolean(TCP_NO_DELAY, false);
     }
 
     @Override
@@ -128,17 +124,17 @@ public int getTcpSendBufferSize() {
 
     @Override
     public int getNettyWriteBufferHighWaterMark() {
-        return WRITE_BUFFER_HIGH_WATER_MARK.get();
+        return clientConfig.getPropertyAsInteger(WRITE_BUFFER_HIGH_WATER_MARK, DEFAULT_WRITE_BUFFER_HIGH_WATER_MARK);
     }
 
     @Override
     public int getNettyWriteBufferLowWaterMark() {
-        return WRITE_BUFFER_LOW_WATER_MARK.get();
+        return clientConfig.getPropertyAsInteger(WRITE_BUFFER_LOW_WATER_MARK, DEFAULT_WRITE_BUFFER_LOW_WATER_MARK);
     }
 
     @Override
     public boolean getNettyAutoRead() {
-        return AUTO_READ.get();
+        return clientConfig.getPropertyAsBoolean(AUTO_READ, false);
     }
 
     @Override

zuul-core/src/test/java/com/netflix/zuul/netty/connectionpool/ConnectionPoolConfigImplTest.java

@@ -0,0 +1,218 @@
+/*
+ * Copyright 2024 Netflix, Inc.
+ *
+ *      Licensed under the Apache License, Version 2.0 (the "License");
+ *      you may not use this file except in compliance with the License.
+ *      You may obtain a copy of the License at
+ *
+ *          http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *      Unless required by applicable law or agreed to in writing, software
+ *      distributed under the License is distributed on an "AS IS" BASIS,
+ *      WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *      See the License for the specific language governing permissions and
+ *      limitations under the License.
+ */
+
+package com.netflix.zuul.netty.connectionpool;
+
+import static com.netflix.zuul.netty.connectionpool.ConnectionPoolConfigImpl.MAX_REQUESTS_PER_CONNECTION;
+import static com.netflix.zuul.netty.connectionpool.ConnectionPoolConfigImpl.PER_SERVER_WATERLINE;
+import static com.netflix.zuul.netty.connectionpool.ConnectionPoolConfigImpl.TCP_KEEP_ALIVE;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.netflix.client.config.DefaultClientConfigImpl;
+import com.netflix.client.config.IClientConfigKey;
+import com.netflix.zuul.origins.OriginName;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * @author Justin Guerra
+ * @since 6/20/24
+ */
+class ConnectionPoolConfigImplTest {
+
+    private ConnectionPoolConfig connectionPoolConfig;
+    private DefaultClientConfigImpl clientConfig;
+
+    @BeforeEach
+    void setup() {
+        OriginName originName = OriginName.fromVip("whatever");
+        clientConfig = DefaultClientConfigImpl.getEmptyConfig();
+        connectionPoolConfig = new ConnectionPoolConfigImpl(originName, clientConfig);
+    }
+
+    @Test
+    void testGetConnectTimeout() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_CONNECT_TIMEOUT, connectionPoolConfig.getConnectTimeout());
+    }
+
+    @Test
+    void testGetConnectTimeoutOverride() {
+        clientConfig.set(IClientConfigKey.Keys.ConnectTimeout, 1000);
+        assertEquals(1000, connectionPoolConfig.getConnectTimeout());
+    }
+
+    @Test
+    void testGetMaxRequestsPerConnection() {
+        assertEquals(
+                ConnectionPoolConfigImpl.DEFAULT_MAX_REQUESTS_PER_CONNECTION,
+                connectionPoolConfig.getMaxRequestsPerConnection());
+    }
+
+    @Test
+    void testGetMaxRequestsPerConnectionOverride() {
+        clientConfig.set(MAX_REQUESTS_PER_CONNECTION, 2000);
+        assertEquals(2000, connectionPoolConfig.getMaxRequestsPerConnection());
+    }
+
+    @Test
+    void testMaxConnectionsPerHost() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_MAX_CONNS_PER_HOST, connectionPoolConfig.maxConnectionsPerHost());
+    }
+
+    @Test
+    void testMaxConnectionsPerHostOverride() {
+        clientConfig.set(IClientConfigKey.Keys.MaxConnectionsPerHost, 60);
+        assertEquals(60, connectionPoolConfig.maxConnectionsPerHost());
+    }
+
+    @Test
+    void testPerServerWaterline() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_PER_SERVER_WATERLINE, connectionPoolConfig.perServerWaterline());
+    }
+
+    @Test
+    void testPerServerWaterlineOverride() {
+        clientConfig.set(PER_SERVER_WATERLINE, 5);
+        assertEquals(5, connectionPoolConfig.perServerWaterline());
+    }
+
+    @Test
+    void testGetIdleTimeout() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_IDLE_TIMEOUT, connectionPoolConfig.getIdleTimeout());
+    }
+
+    @Test
+    void testGetIdleTimeoutOverride() {
+        clientConfig.set(IClientConfigKey.Keys.ConnIdleEvictTimeMilliSeconds, 70000);
+        assertEquals(70000, connectionPoolConfig.getIdleTimeout());
+    }
+
+    @Test
+    void testGetTcpKeepAlive() {
+        assertFalse(connectionPoolConfig.getTcpKeepAlive());
+    }
+
+    @Test
+    void testGetTcpKeepAliveOverride() {
+        clientConfig.set(TCP_KEEP_ALIVE, true);
+        assertTrue(connectionPoolConfig.getTcpKeepAlive());
+    }
+
+    @Test
+    void testGetTcpNoDelay() {
+        assertFalse(connectionPoolConfig.getTcpNoDelay());
+    }
+
+    @Test
+    void testGetTcpNoDelayOverride() {
+        clientConfig.set(ConnectionPoolConfigImpl.TCP_NO_DELAY, true);
+        assertTrue(connectionPoolConfig.getTcpNoDelay());
+    }
+
+    @Test
+    void testGetTcpReceiveBufferSize() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_BUFFER_SIZE, connectionPoolConfig.getTcpReceiveBufferSize());
+    }
+
+    @Test
+    void testGetTcpReceiveBufferSizeOverride() {
+        clientConfig.set(IClientConfigKey.Keys.ReceiveBufferSize, 40000);
+        assertEquals(40000, connectionPoolConfig.getTcpReceiveBufferSize());
+    }
+
+    @Test
+    void testGetTcpSendBufferSize() {
+        assertEquals(ConnectionPoolConfigImpl.DEFAULT_BUFFER_SIZE, connectionPoolConfig.getTcpSendBufferSize());
+    }
+
+    @Test
+    void testGetTcpSendBufferSizeOverride() {
+        clientConfig.set(IClientConfigKey.Keys.SendBufferSize, 40000);
+        assertEquals(40000, connectionPoolConfig.getTcpSendBufferSize());
+    }
+
+    @Test
+    void testGetNettyWriteBufferHighWaterMark() {
+        assertEquals(
+                ConnectionPoolConfigImpl.DEFAULT_WRITE_BUFFER_HIGH_WATER_MARK,
+                connectionPoolConfig.getNettyWriteBufferHighWaterMark());
+    }
+
+    @Test
+    void testGetNettyWriteBufferHighWaterMarkOverride() {
+        clientConfig.set(ConnectionPoolConfigImpl.WRITE_BUFFER_HIGH_WATER_MARK, 40000);
+        assertEquals(40000, connectionPoolConfig.getNettyWriteBufferHighWaterMark());
+    }
+
+    @Test
+    void testGetNettyWriteBufferLowWaterMark() {
+        assertEquals(
+                ConnectionPoolConfigImpl.DEFAULT_WRITE_BUFFER_LOW_WATER_MARK,
+                connectionPoolConfig.getNettyWriteBufferLowWaterMark());
+    }
+
+    @Test
+    void testGetNettyWriteBufferLowWaterMarkOverride() {
+        clientConfig.set(ConnectionPoolConfigImpl.WRITE_BUFFER_LOW_WATER_MARK, 10000);
+        assertEquals(10000, connectionPoolConfig.getNettyWriteBufferLowWaterMark());
+    }
+
+    @Test
+    void testGetNettyAutoRead() {
+        assertFalse(connectionPoolConfig.getNettyAutoRead());
+    }
+
+    @Test
+    void testGetNettyAutoReadOverride() {
+        clientConfig.set(ConnectionPoolConfigImpl.AUTO_READ, true);
+        assertTrue(connectionPoolConfig.getNettyAutoRead());
+    }
+
+    @Test
+    void testIsSecure() {
+        assertFalse(connectionPoolConfig.isSecure());
+    }
+
+    @Test
+    void testIsSecureOverride() {
+        clientConfig.set(IClientConfigKey.Keys.IsSecure, true);
+        assertTrue(connectionPoolConfig.isSecure());
+    }
+
+    @Test
+    void testUseIPAddrForServer() {
+        assertTrue(connectionPoolConfig.useIPAddrForServer());
+    }
+
+    @Test
+    void testUseIPAddrForServerOverride() {
+        clientConfig.set(IClientConfigKey.Keys.UseIPAddrForServer, false);
+        assertFalse(connectionPoolConfig.useIPAddrForServer());
+    }
+
+    @Test
+    void testIsCloseOnCircuitBreakerEnabled() {
+        assertTrue(connectionPoolConfig.isCloseOnCircuitBreakerEnabled());
+    }
+
+    @Test
+    void testIsCloseOnCircuitBreakerEnabledOverride() {
+        clientConfig.set(ConnectionPoolConfigImpl.CLOSE_ON_CIRCUIT_BREAKER, false);
+        assertFalse(connectionPoolConfig.isCloseOnCircuitBreakerEnabled());
+    }
+}