Skip to content

Eliminate mutable field anti-patterns #777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Eliminate mutable field anti-patterns #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsJdbcMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.gateway.ha.clustermonitor;

import com.google.common.collect.ImmutableMap;
import com.google.common.util.concurrent.SimpleTimeLimiter;
import io.airlift.log.Logger;
import io.airlift.units.Duration;
Expand All @@ -39,7 +40,7 @@ public class ClusterStatsJdbcMonitor
{
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;
private final Duration queryTimeout;

private static final String STATE_QUERY = "SELECT state, COUNT(*) as count "
Expand All @@ -49,15 +50,15 @@ 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()));
ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
propertiesBuilder.put("user", backendStateConfiguration.getUsername());
propertiesBuilder.put("password", backendStateConfiguration.getPassword());
// 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");
propertiesBuilder.put("explicitPrepare", "false");
}
properties = propertiesBuilder.build();
queryTimeout = monitorConfiguration.getQueryTimeout();
log.info("state check configured");
}
Expand All @@ -68,23 +69,32 @@ 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();
// Remove any existing SSL property to avoid confusion
for (String key : properties.keySet()) {
if (!key.equalsIgnoreCase("SSL")) {
connectionProperties.setProperty(key, properties.get(key));
}
}
Comment on lines +81 to +86
Copy link
Member

@oneonestar oneonestar Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since properties will not have "SSL" anymore, we don't need this logic.

// Set SSL config based on url protocol, always taking precedence
connectionProperties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
}
catch (MalformedURLException e) {
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, properties.get("user"));
statement.setQueryTimeout((int) queryTimeout.roundTo(SECONDS));
Map<String, Integer> partialState = new HashMap<>();
ResultSet rs = statement.executeQuery();
Expand Down
11 changes: 0 additions & 11 deletions gateway-ha/src/main/java/io/trino/gateway/ha/config/BackendStateConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ public class BackendStateConfiguration
{
private String username;
private String password = "";
private Boolean ssl = false;
private boolean xForwardedProtoHeader;

public BackendStateConfiguration() {}
Expand All @@ -42,16 +41,6 @@ public void setPassword(String password)
this.password = password;
}

public Boolean getSsl()
{
return this.ssl;
}

public void setSsl(Boolean ssl)
{
this.ssl = ssl;
}

public boolean getXForwardedProtoHeader()
{
return xForwardedProtoHeader;
Expand Down
3 changes: 1 addition & 2 deletions gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ public class GatewayWebAppResource
private final ResourceGroupsManager resourceGroupsManager;
private final boolean isRulesEngineEnabled;
private final RulesType ruleType;
// TODO Avoid putting mutable objects in fields
private final UIConfiguration uiConfiguration;
private final RoutingRulesManager routingRulesManager;

Expand All @@ -95,7 +94,7 @@ 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.uiConfiguration = requireNonNull(configuration.getUiConfiguration(), "uiConfiguration is null");
this.routingRulesManager = requireNonNull(routingRulesManager, "routingRulesManager is null");
RoutingRulesConfiguration routingRules = configuration.getRoutingRules();
isRulesEngineEnabled = routingRules.isRulesEngineEnabled();
Expand Down