Eliminate mutable field anti-patterns and improve error handling

Author: Akanksha-kedia

gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsJdbcMonitor.java

@@ -32,14 +32,17 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeoutException;
 
+import com.google.common.collect.ImmutableMap;
+
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 public class ClusterStatsJdbcMonitor
         implements ClusterStatsMonitor
 {
     private static final Logger log = Logger.get(ClusterStatsJdbcMonitor.class);
 
-    private final Properties properties; // TODO Avoid using a mutable field
+    private final ImmutableMap<String, String> properties; // Replaced mutable Properties with ImmutableMap
+    private final JdbcMonitorConfiguration jdbcConfig;
     private final Duration queryTimeout;
 
     private static final String STATE_QUERY = "SELECT state, COUNT(*) as count "
@@ -49,16 +52,17 @@ public class ClusterStatsJdbcMonitor
 
     public ClusterStatsJdbcMonitor(BackendStateConfiguration backendStateConfiguration, MonitorConfiguration monitorConfiguration)
     {
-        properties = new Properties();
-        properties.setProperty("user", backendStateConfiguration.getUsername());
-        properties.setProperty("password", backendStateConfiguration.getPassword());
-        properties.setProperty("SSL", String.valueOf(backendStateConfiguration.getSsl()));
-        // explicitPrepare is a valid property for Trino versions >= 431. To avoid compatibility
-        // issues with versions < 431, this property is left unset when explicitPrepare=true, which is the default
-        if (!monitorConfiguration.isExplicitPrepare()) {
-            properties.setProperty("explicitPrepare", "false");
-        }
+        this.jdbcConfig = JdbcMonitorConfiguration.from(backendStateConfiguration, monitorConfiguration);
         queryTimeout = monitorConfiguration.getQueryTimeout();
+
+        // Create immutable properties map to avoid mutable field anti-pattern
+        Properties baseProperties = jdbcConfig.toProperties();
+        ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+        for (String key : baseProperties.stringPropertyNames()) {
+            propertiesBuilder.put(key, baseProperties.getProperty(key));
+        }
+        this.properties = propertiesBuilder.build();
+
         log.info("state check configured");
     }
 
@@ -68,24 +72,28 @@ public ClusterStats monitor(ProxyBackendConfiguration backend)
         String url = backend.getProxyTo();
         ClusterStats.Builder clusterStats = ClusterStatsMonitor.getClusterStatsBuilder(backend);
         String jdbcUrl;
+        Properties connectionProperties;
         try {
             URL parsedUrl = new URL(url);
             jdbcUrl = String
                     .format("jdbc:trino://%s:%s/system",
                             parsedUrl.getHost(),
                             parsedUrl.getPort() == -1 ? parsedUrl.getDefaultPort() : parsedUrl.getPort());
-            // automatically set ssl config based on url protocol
-            properties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
+
+            // Create connection properties from immutable map
+            connectionProperties = new Properties();
+            connectionProperties.putAll(properties);
+            connectionProperties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
         }
         catch (MalformedURLException e) {
-            log.error("could not parse backend url %s ", url);
-            return clusterStats.build(); // TODO Invalid configuration should fail
+            log.error("Invalid backend URL configuration: %s", url);
+            throw new IllegalArgumentException("Invalid backend URL: " + url, e);
         }
 
-        try (Connection conn = DriverManager.getConnection(jdbcUrl, properties);
+        try (Connection conn = DriverManager.getConnection(jdbcUrl, connectionProperties);
                 PreparedStatement statement = SimpleTimeLimiter.create(Executors.newSingleThreadExecutor()).callWithTimeout(
                         () -> conn.prepareStatement(STATE_QUERY), 10, SECONDS)) {
-            statement.setString(1, (String) properties.get("user"));
+            statement.setString(1, jdbcConfig.getUsername());
             statement.setQueryTimeout((int) queryTimeout.roundTo(SECONDS));
             Map<String, Integer> partialState = new HashMap<>();
             ResultSet rs = statement.executeQuery();

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java

@@ -94,8 +94,11 @@ public GatewayWebAppResource(
         this.queryHistoryManager = requireNonNull(queryHistoryManager, "queryHistoryManager is null");
         this.backendStateManager = requireNonNull(backendStateManager, "backendStateManager is null");
         this.resourceGroupsManager = requireNonNull(resourceGroupsManager, "resourceGroupsManager is null");
-        this.uiConfiguration = configuration.getUiConfiguration();
         this.routingRulesManager = requireNonNull(routingRulesManager, "routingRulesManager is null");
+
+        // UI configuration is immutable during application lifetime, no need to copy
+        this.uiConfiguration = configuration.getUiConfiguration();
+
         RoutingRulesConfiguration routingRules = configuration.getRoutingRules();
         isRulesEngineEnabled = routingRules.isRulesEngineEnabled();
         ruleType = routingRules.getRulesType();