Skip to content

Commit b47c06d

Browse files
Akanksha-kediaAkanksha-kedia
committed
Eliminate mutable field anti-patterns and improve error handling
1 parent 89bbae8 commit b47c06d

File tree

4 files changed

+242
-21
lines changed

4 files changed

+242
-21
lines changed

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

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import io.airlift.log.Logger;
1818
import io.airlift.units.Duration;
1919
import io.trino.gateway.ha.config.BackendStateConfiguration;
20+
import io.trino.gateway.ha.config.JdbcMonitorConfiguration;
2021
import io.trino.gateway.ha.config.MonitorConfiguration;
2122
import io.trino.gateway.ha.config.ProxyBackendConfiguration;
2223

@@ -39,7 +40,7 @@ public class ClusterStatsJdbcMonitor
3940
{
4041
private static final Logger log = Logger.get(ClusterStatsJdbcMonitor.class);
4142

42-
private final Properties properties; // TODO Avoid using a mutable field
43+
private final JdbcMonitorConfiguration jdbcConfig;
4344
private final Duration queryTimeout;
4445

4546
private static final String STATE_QUERY = "SELECT state, COUNT(*) as count "
@@ -49,15 +50,7 @@ public class ClusterStatsJdbcMonitor
4950

5051
public ClusterStatsJdbcMonitor(BackendStateConfiguration backendStateConfiguration, MonitorConfiguration monitorConfiguration)
5152
{
52-
properties = new Properties();
53-
properties.setProperty("user", backendStateConfiguration.getUsername());
54-
properties.setProperty("password", backendStateConfiguration.getPassword());
55-
properties.setProperty("SSL", String.valueOf(backendStateConfiguration.getSsl()));
56-
// explicitPrepare is a valid property for Trino versions >= 431. To avoid compatibility
57-
// issues with versions < 431, this property is left unset when explicitPrepare=true, which is the default
58-
if (!monitorConfiguration.isExplicitPrepare()) {
59-
properties.setProperty("explicitPrepare", "false");
60-
}
53+
this.jdbcConfig = JdbcMonitorConfiguration.from(backendStateConfiguration, monitorConfiguration);
6154
queryTimeout = monitorConfiguration.getQueryTimeout();
6255
log.info("state check configured");
6356
}
@@ -68,24 +61,26 @@ public ClusterStats monitor(ProxyBackendConfiguration backend)
6861
String url = backend.getProxyTo();
6962
ClusterStats.Builder clusterStats = ClusterStatsMonitor.getClusterStatsBuilder(backend);
7063
String jdbcUrl;
64+
Properties connectionProperties;
7165
try {
7266
URL parsedUrl = new URL(url);
7367
jdbcUrl = String
7468
.format("jdbc:trino://%s:%s/system",
7569
parsedUrl.getHost(),
7670
parsedUrl.getPort() == -1 ? parsedUrl.getDefaultPort() : parsedUrl.getPort());
77-
// automatically set ssl config based on url protocol
78-
properties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
71+
// Create connection properties with SSL setting based on URL protocol
72+
connectionProperties = jdbcConfig.toProperties();
73+
connectionProperties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
7974
}
8075
catch (MalformedURLException e) {
81-
log.error("could not parse backend url %s ", url);
82-
return clusterStats.build(); // TODO Invalid configuration should fail
76+
log.error("Invalid backend URL configuration: %s", url);
77+
throw new IllegalArgumentException("Invalid backend URL: " + url, e);
8378
}
8479

85-
try (Connection conn = DriverManager.getConnection(jdbcUrl, properties);
80+
try (Connection conn = DriverManager.getConnection(jdbcUrl, connectionProperties);
8681
PreparedStatement statement = SimpleTimeLimiter.create(Executors.newSingleThreadExecutor()).callWithTimeout(
8782
() -> conn.prepareStatement(STATE_QUERY), 10, SECONDS)) {
88-
statement.setString(1, (String) properties.get("user"));
83+
statement.setString(1, jdbcConfig.getUsername());
8984
statement.setQueryTimeout((int) queryTimeout.roundTo(SECONDS));
9085
Map<String, Integer> partialState = new HashMap<>();
9186
ResultSet rs = statement.executeQuery();

‎gateway-ha/src/main/java/io/trino/gateway/ha/config/ImmutableUIConfiguration.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package io.trino.gateway.ha.config;
15+
16+
import com.fasterxml.jackson.annotation.JsonCreator;
17+
import com.fasterxml.jackson.annotation.JsonProperty;
18+
import com.google.common.collect.ImmutableList;
19+
20+
import java.util.List;
21+
import java.util.Objects;
22+
23+
import static com.google.common.base.MoreObjects.toStringHelper;
24+
25+
/**
26+
* Immutable wrapper for UI configuration to ensure thread safety.
27+
* Replaces mutable UIConfiguration objects in fields to prevent accidental modification.
28+
*/
29+
public class ImmutableUIConfiguration
30+
{
31+
private final ImmutableList<String> disablePages;
32+
33+
@JsonCreator
34+
public ImmutableUIConfiguration(@JsonProperty("disablePages") List<String> disablePages)
35+
{
36+
this.disablePages = disablePages != null ?
37+
ImmutableList.copyOf(disablePages) :
38+
ImmutableList.of();
39+
}
40+
41+
/**
42+
* Creates an immutable copy from a mutable UIConfiguration.
43+
* This factory method provides a safe way to convert mutable configurations.
44+
*/
45+
public static ImmutableUIConfiguration copyOf(UIConfiguration original)
46+
{
47+
return new ImmutableUIConfiguration(original != null ? original.getDisablePages() : null);
48+
}
49+
50+
@JsonProperty
51+
public List<String> getDisablePages()
52+
{
53+
return disablePages;
54+
}
55+
56+
@Override
57+
public boolean equals(Object o)
58+
{
59+
if (this == o) {
60+
return true;
61+
}
62+
if (o == null || getClass() != o.getClass()) {
63+
return false;
64+
}
65+
ImmutableUIConfiguration that = (ImmutableUIConfiguration) o;
66+
return Objects.equals(disablePages, that.disablePages);
67+
}
68+
69+
@Override
70+
public int hashCode()
71+
{
72+
return Objects.hash(disablePages);
73+
}
74+
75+
@Override
76+
public String toString()
77+
{
78+
return toStringHelper(this)
79+
.add("disablePages", disablePages)
80+
.toString();
81+
}
82+
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/config/JdbcMonitorConfiguration.java‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package io.trino.gateway.ha.config;
15+
16+
import com.fasterxml.jackson.annotation.JsonCreator;
17+
import com.fasterxml.jackson.annotation.JsonProperty;
18+
19+
import java.util.Objects;
20+
import java.util.Properties;
21+
22+
import static com.google.common.base.MoreObjects.toStringHelper;
23+
24+
/**
25+
* Immutable configuration for JDBC monitoring connections.
26+
* Replaces the mutable Properties object to ensure thread safety.
27+
*/
28+
public class JdbcMonitorConfiguration
29+
{
30+
private final String username;
31+
private final String password;
32+
private final boolean ssl;
33+
private final boolean explicitPrepare;
34+
35+
@JsonCreator
36+
public JdbcMonitorConfiguration(
37+
@JsonProperty("username") String username,
38+
@JsonProperty("password") String password,
39+
@JsonProperty("ssl") boolean ssl,
40+
@JsonProperty("explicitPrepare") boolean explicitPrepare)
41+
{
42+
this.username = username;
43+
this.password = password;
44+
this.ssl = ssl;
45+
this.explicitPrepare = explicitPrepare;
46+
}
47+
48+
/**
49+
* Creates a JdbcMonitorConfiguration from BackendStateConfiguration and MonitorConfiguration.
50+
* This factory method encapsulates the logic for building JDBC properties.
51+
*/
52+
public static JdbcMonitorConfiguration from(
53+
io.trino.gateway.ha.config.BackendStateConfiguration backendStateConfiguration,
54+
io.trino.gateway.ha.config.MonitorConfiguration monitorConfiguration)
55+
{
56+
return new JdbcMonitorConfiguration(
57+
backendStateConfiguration.getUsername(),
58+
backendStateConfiguration.getPassword(),
59+
backendStateConfiguration.getSsl(),
60+
monitorConfiguration.isExplicitPrepare());
61+
}
62+
63+
/**
64+
* Converts this configuration to a Properties object for JDBC driver compatibility.
65+
* This method provides backward compatibility while maintaining immutability.
66+
*/
67+
public Properties toProperties()
68+
{
69+
Properties properties = new Properties();
70+
if (username != null) {
71+
properties.setProperty("user", username);
72+
}
73+
if (password != null) {
74+
properties.setProperty("password", password);
75+
}
76+
properties.setProperty("SSL", String.valueOf(ssl));
77+
78+
// explicitPrepare is a valid property for Trino versions >= 431. To avoid compatibility
79+
// issues with versions < 431, this property is left unset when explicitPrepare=true, which is the default
80+
if (!explicitPrepare) {
81+
properties.setProperty("explicitPrepare", "false");
82+
}
83+
84+
return properties;
85+
}
86+
87+
@JsonProperty
88+
public String getUsername()
89+
{
90+
return username;
91+
}
92+
93+
@JsonProperty
94+
public String getPassword()
95+
{
96+
return password;
97+
}
98+
99+
@JsonProperty
100+
public boolean isSsl()
101+
{
102+
return ssl;
103+
}
104+
105+
@JsonProperty
106+
public boolean isExplicitPrepare()
107+
{
108+
return explicitPrepare;
109+
}
110+
111+
@Override
112+
public boolean equals(Object o)
113+
{
114+
if (this == o) {
115+
return true;
116+
}
117+
if (o == null || getClass() != o.getClass()) {
118+
return false;
119+
}
120+
JdbcMonitorConfiguration that = (JdbcMonitorConfiguration) o;
121+
return ssl == that.ssl &&
122+
explicitPrepare == that.explicitPrepare &&
123+
Objects.equals(username, that.username) &&
124+
Objects.equals(password, that.password);
125+
}
126+
127+
@Override
128+
public int hashCode()
129+
{
130+
return Objects.hash(username, password, ssl, explicitPrepare);
131+
}
132+
133+
@Override
134+
public String toString()
135+
{
136+
return toStringHelper(this)
137+
.add("username", username)
138+
.add("ssl", ssl)
139+
.add("explicitPrepare", explicitPrepare)
140+
.toString();
141+
}
142+
}

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@
1717
import com.google.inject.Inject;
1818
import io.trino.gateway.ha.clustermonitor.ClusterStats;
1919
import io.trino.gateway.ha.config.HaGatewayConfiguration;
20+
import io.trino.gateway.ha.config.ImmutableUIConfiguration;
2021
import io.trino.gateway.ha.config.ProxyBackendConfiguration;
2122
import io.trino.gateway.ha.config.RoutingRulesConfiguration;
2223
import io.trino.gateway.ha.config.RulesType;
23-
import io.trino.gateway.ha.config.UIConfiguration;
2424
import io.trino.gateway.ha.domain.Result;
2525
import io.trino.gateway.ha.domain.RoutingRule;
2626
import io.trino.gateway.ha.domain.TableData;
@@ -77,8 +77,7 @@ public class GatewayWebAppResource
7777
private final ResourceGroupsManager resourceGroupsManager;
7878
private final boolean isRulesEngineEnabled;
7979
private final RulesType ruleType;
80-
// TODO Avoid putting mutable objects in fields
81-
private final UIConfiguration uiConfiguration;
80+
private final ImmutableUIConfiguration immutableUiConfiguration;
8281
private final RoutingRulesManager routingRulesManager;
8382

8483
@Inject
@@ -94,8 +93,11 @@ public GatewayWebAppResource(
9493
this.queryHistoryManager = requireNonNull(queryHistoryManager, "queryHistoryManager is null");
9594
this.backendStateManager = requireNonNull(backendStateManager, "backendStateManager is null");
9695
this.resourceGroupsManager = requireNonNull(resourceGroupsManager, "resourceGroupsManager is null");
97-
this.uiConfiguration = configuration.getUiConfiguration();
9896
this.routingRulesManager = requireNonNull(routingRulesManager, "routingRulesManager is null");
97+
98+
// Create immutable copy of UI configuration to ensure thread safety
99+
this.immutableUiConfiguration = ImmutableUIConfiguration.copyOf(configuration.getUiConfiguration());
100+
99101
RoutingRulesConfiguration routingRules = configuration.getRoutingRules();
100102
isRulesEngineEnabled = routingRules.isRulesEngineEnabled();
101103
ruleType = routingRules.getRulesType();
@@ -479,6 +481,6 @@ public Response updateRoutingRules(RoutingRule routingRule)
479481
@Path("/getUIConfiguration")
480482
public Response getUIConfiguration()
481483
{
482-
return Response.ok(Result.ok(uiConfiguration)).build();
484+
return Response.ok(Result.ok(immutableUiConfiguration)).build();
483485
}
484486
}

0 commit comments

Comments
 (0)