Skip to content

Commit c13a8db

Browse files
felicity3786felicity3786
committed
address comments
1 parent 8a6ab09 commit c13a8db

15 files changed

+158
-137
lines changed

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

Lines changed: 42 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@
1313
*/
1414
package io.trino.gateway.ha.config;
1515

16-
import java.util.Locale;
16+
import java.sql.Driver;
17+
import java.sql.DriverManager;
18+
import java.sql.SQLException;
19+
import java.util.Enumeration;
1720

1821
public class DataStoreConfiguration
1922
{
@@ -23,11 +26,11 @@ public class DataStoreConfiguration
2326
private String driver;
2427
private Integer queryHistoryHoursRetention = 4;
2528
private boolean runMigrationsEnabled = true;
26-
private DataStoreBackend dataStoreBackendType;
29+
private DataStoreType dataStoreType;
2730

2831
// TODO: Refactor to decouple DataStoreConfiguration from a specific
2932
// database implementation after adopting the Airlift configuration framework (https://github.com/trinodb/trino-gateway/issues/378)
30-
private MysqlConfiguration mysqlConfiguration = new MysqlConfiguration();
33+
private MySqlConfiguration mySqlConfiguration = new MySqlConfiguration();
3134

3235
public DataStoreConfiguration(String jdbcUrl, String user, String password, String driver, Integer queryHistoryHoursRetention, boolean runMigrationsEnabled)
3336
{
@@ -101,43 +104,55 @@ public void setRunMigrationsEnabled(boolean runMigrationsEnabled)
101104
this.runMigrationsEnabled = runMigrationsEnabled;
102105
}
103106

104-
public MysqlConfiguration getMysqlConfiguration()
107+
public MySqlConfiguration getMySqlConfiguration()
105108
{
106-
return mysqlConfiguration;
109+
return mySqlConfiguration;
107110
}
108111

109-
public void setMysqlConfiguration(MysqlConfiguration mysqlConfig)
112+
public void setMysqlConfiguration(MySqlConfiguration mysqlConfig)
110113
{
111-
this.mysqlConfiguration = mysqlConfig;
114+
this.mySqlConfiguration = mysqlConfig;
112115
}
113116

114-
public DataStoreBackend getDataStoreBackendType()
117+
public DataStoreType getDataStoreType()
115118
{
116-
if (dataStoreBackendType != null) {
117-
return dataStoreBackendType;
119+
if (dataStoreType != null) {
120+
return dataStoreType;
121+
}
122+
if (jdbcUrl == null) {
123+
throw new IllegalStateException("jdbcUrl must be set");
118124
}
119125

120-
if (jdbcUrl != null) {
121-
String url = jdbcUrl.toLowerCase(Locale.ROOT);
122-
if (url.startsWith("jdbc:oracle:")) {
123-
return DataStoreBackend.ORACLE;
124-
}
125-
if (url.startsWith("jdbc:mysql:")) {
126-
return DataStoreBackend.MYSQL;
127-
}
128-
if (url.startsWith("jdbc:postgresql:")) {
129-
return DataStoreBackend.POSTGRES;
130-
}
131-
if (url.startsWith("jdbc:h2:")) {
132-
return DataStoreBackend.H2;
126+
try {
127+
Enumeration<Driver> drivers = DriverManager.getDrivers();
128+
while (drivers.hasMoreElements()) {
129+
Driver driver = drivers.nextElement();
130+
if (driver.acceptsURL(jdbcUrl)) {
131+
String driverClass = driver.getClass().getName();
132+
if (driverClass.contains("oracle")) {
133+
return DataStoreType.ORACLE;
134+
}
135+
if (driverClass.contains("mysql")) {
136+
return DataStoreType.MYSQL;
137+
}
138+
if (driverClass.contains("postgresql")) {
139+
return DataStoreType.POSTGRES;
140+
}
141+
if (driverClass.contains("h2")) {
142+
return DataStoreType.H2;
143+
}
144+
break;
145+
}
133146
}
134147
}
135-
136-
throw new IllegalStateException("Cannot infer DataStoreBackend from jdbcUrl: " + jdbcUrl);
148+
catch (SQLException e) {
149+
throw new RuntimeException("Error enumerating JDBC drivers", e);
150+
}
151+
throw new IllegalStateException("Unable to infer DataStoreType for URL: " + jdbcUrl);
137152
}
138153

139-
public void setDataStoreBackendType(DataStoreBackend backendType)
154+
public void setDataStoreType(DataStoreType backendType)
140155
{
141-
this.dataStoreBackendType = backendType;
156+
this.dataStoreType = backendType;
142157
}
143158
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreBackend.java‎ renamed to ‎gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreType.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
*/
1414
package io.trino.gateway.ha.config;
1515

16-
public enum DataStoreBackend {
16+
public enum DataStoreType {
1717
ORACLE,
1818
MYSQL,
1919
POSTGRES,

‎gateway-ha/src/main/java/io/trino/gateway/ha/config/MysqlConfiguration.java‎ renamed to ‎gateway-ha/src/main/java/io/trino/gateway/ha/config/MySqlConfiguration.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
/**
1919
* Configuration for MySQL SSL (client cert and truststore settings).
2020
*/
21-
public class MysqlConfiguration
21+
public class MySqlConfiguration
2222
{
2323
private SslMode sslMode = SslMode.DISABLED;
2424
private String clientCertificateKeyStoreUrl;
@@ -32,7 +32,7 @@ public SslMode getSslMode()
3232
return sslMode;
3333
}
3434

35-
public MysqlConfiguration setSslMode(SslMode sslMode)
35+
public MySqlConfiguration setSslMode(SslMode sslMode)
3636
{
3737
this.sslMode = sslMode;
3838
return this;
@@ -43,7 +43,7 @@ public String getClientCertificateKeyStoreUrl()
4343
return clientCertificateKeyStoreUrl;
4444
}
4545

46-
public MysqlConfiguration setClientCertificateKeyStoreUrl(String url)
46+
public MySqlConfiguration setClientCertificateKeyStoreUrl(String url)
4747
{
4848
this.clientCertificateKeyStoreUrl = url;
4949
return this;
@@ -54,7 +54,7 @@ public String getClientCertificateKeyStorePassword()
5454
return clientCertificateKeyStorePassword;
5555
}
5656

57-
public MysqlConfiguration setClientCertificateKeyStorePassword(String password)
57+
public MySqlConfiguration setClientCertificateKeyStorePassword(String password)
5858
{
5959
this.clientCertificateKeyStorePassword = password;
6060
return this;
@@ -65,7 +65,7 @@ public String getClientCertificateKeyStoreType()
6565
return clientCertificateKeyStoreType;
6666
}
6767

68-
public MysqlConfiguration setClientCertificateKeyStoreType(String type)
68+
public MySqlConfiguration setClientCertificateKeyStoreType(String type)
6969
{
7070
this.clientCertificateKeyStoreType = type;
7171
return this;
@@ -76,7 +76,7 @@ public String getTrustCertificateKeyStoreUrl()
7676
return trustCertificateKeyStoreUrl;
7777
}
7878

79-
public MysqlConfiguration setTrustCertificateKeyStoreUrl(String url)
79+
public MySqlConfiguration setTrustCertificateKeyStoreUrl(String url)
8080
{
8181
this.trustCertificateKeyStoreUrl = url;
8282
return this;
@@ -87,7 +87,7 @@ public String getTrustCertificateKeyStorePassword()
8787
return trustCertificateKeyStorePassword;
8888
}
8989

90-
public MysqlConfiguration setTrustCertificateKeyStorePassword(String password)
90+
public MySqlConfiguration setTrustCertificateKeyStorePassword(String password)
9191
{
9292
this.trustCertificateKeyStorePassword = password;
9393
return this;

‎gateway-ha/src/main/java/io/trino/gateway/ha/module/QueryCountBasedRouterProvider.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,21 @@
2121
import io.trino.gateway.ha.router.QueryHistoryManager;
2222
import io.trino.gateway.ha.router.RoutingManager;
2323

24+
import static java.util.Objects.requireNonNull;
25+
2426
public class QueryCountBasedRouterProvider
2527
extends RouterBaseModule
2628
{
27-
public QueryCountBasedRouterProvider(HaGatewayConfiguration configuration)
28-
{
29-
super(configuration);
30-
}
29+
public QueryCountBasedRouterProvider(HaGatewayConfiguration configuration) {}
3130

3231
@Provides
3332
@Singleton
3433
public RoutingManager provideRoutingManager(
3534
GatewayBackendManager gatewayBackendManager,
3635
QueryHistoryManager queryHistoryManager)
3736
{
38-
return new QueryCountBasedRouter(gatewayBackendManager, queryHistoryManager);
37+
return new QueryCountBasedRouter(
38+
requireNonNull(gatewayBackendManager, "gatewayBackendManager is null"),
39+
requireNonNull(queryHistoryManager, "queryHistoryManager is null"));
3940
}
4041
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/module/RouterBaseModule.java‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import com.google.inject.TypeLiteral;
2020
import io.trino.gateway.ha.config.DataStoreConfiguration;
2121
import io.trino.gateway.ha.config.HaGatewayConfiguration;
22-
import io.trino.gateway.ha.persistence.BasicJdbcPropertiesProvider;
22+
import io.trino.gateway.ha.persistence.DefaultJdbcPropertiesProvider;
2323
import io.trino.gateway.ha.persistence.JdbcConnectionManager;
2424
import io.trino.gateway.ha.persistence.JdbcPropertiesProvider;
2525
import io.trino.gateway.ha.persistence.JdbcPropertiesProviderFactory;
@@ -39,41 +39,38 @@
3939
public class RouterBaseModule
4040
extends AbstractModule
4141
{
42-
private final HaGatewayConfiguration configuration;
43-
44-
public RouterBaseModule(HaGatewayConfiguration configuration)
45-
{
46-
this.configuration = configuration;
47-
}
48-
4942
@Override
5043
protected void configure()
5144
{
52-
bind(HaGatewayConfiguration.class).toInstance(configuration);
53-
bind(DataStoreConfiguration.class)
54-
.toInstance(configuration.getDataStore());
55-
5645
bind(ResourceGroupsManager.class).to(HaResourceGroupsManager.class);
5746
bind(GatewayBackendManager.class).to(HaGatewayManager.class);
5847
bind(QueryHistoryManager.class).to(HaQueryHistoryManager.class);
5948

6049
bind(new TypeLiteral<List<JdbcPropertiesProvider>>() {}).toInstance(List.of(
6150
new MySqlJdbcPropertiesProvider(),
62-
new BasicJdbcPropertiesProvider()));
51+
new DefaultJdbcPropertiesProvider()));
6352

6453
bind(JdbcPropertiesProviderFactory.class).in(Singleton.class);
6554
}
6655

56+
@Provides
57+
@Singleton
58+
public DataStoreConfiguration provideDataStoreConfiguration(
59+
HaGatewayConfiguration configuration)
60+
{
61+
return configuration.getDataStore();
62+
}
63+
6764
@Provides
6865
@Singleton
6966
public Jdbi provideJdbi(
7067
DataStoreConfiguration configuration,
7168
JdbcPropertiesProviderFactory providerFactory)
7269
{
73-
Properties props = providerFactory
70+
Properties properties = providerFactory
7471
.forConfig(configuration)
7572
.getProperties(configuration);
76-
Jdbi jdbi = Jdbi.create(configuration.getJdbcUrl(), props);
73+
Jdbi jdbi = Jdbi.create(configuration.getJdbcUrl(), properties);
7774
jdbi.installPlugin(new SqlObjectPlugin());
7875
return jdbi;
7976
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/module/StochasticRoutingManagerProvider.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,7 @@
2424
public class StochasticRoutingManagerProvider
2525
extends RouterBaseModule
2626
{
27-
public StochasticRoutingManagerProvider(HaGatewayConfiguration configuration)
28-
{
29-
super(configuration);
30-
}
27+
public StochasticRoutingManagerProvider(HaGatewayConfiguration configuration) {}
3128

3229
@Provides
3330
@Singleton

‎gateway-ha/src/main/java/io/trino/gateway/ha/persistence/BasicJdbcPropertiesProvider.java‎ renamed to ‎gateway-ha/src/main/java/io/trino/gateway/ha/persistence/DefaultJdbcPropertiesProvider.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
* <p>If a more specific provider (e.g., for MySQL, Oracle, etc.) supports the configuration,
2828
* it should be preferred over this basic fallback.
2929
*/
30-
public class BasicJdbcPropertiesProvider
30+
public class DefaultJdbcPropertiesProvider
3131
implements JdbcPropertiesProvider
3232
{
3333
@Override
@@ -39,9 +39,9 @@ public boolean supports(DataStoreConfiguration configuration)
3939
@Override
4040
public Properties getProperties(DataStoreConfiguration configuration)
4141
{
42-
Properties props = new Properties();
43-
props.setProperty("user", configuration.getUser());
44-
props.setProperty("password", configuration.getPassword());
45-
return props;
42+
Properties properties = new Properties();
43+
properties.setProperty("user", configuration.getUser());
44+
properties.setProperty("password", configuration.getPassword());
45+
return properties;
4646
}
4747
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ public Jdbi getJdbi(@Nullable String routingGroupDatabase)
6161
if (routingGroupDatabase == null) {
6262
return jdbi;
6363
}
64-
Properties props = jdbcPropertiesProvider.getProperties(configuration);
64+
Properties properties = jdbcPropertiesProvider.getProperties(configuration);
6565

66-
return Jdbi.create(buildJdbcUrl(routingGroupDatabase), props)
66+
return Jdbi.create(buildJdbcUrl(routingGroupDatabase), properties)
6767
.installPlugin(new SqlObjectPlugin())
6868
.registerRowMapper(new RecordAndAnnotatedConstructorMapper());
6969
}

‎gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcPropertiesProviderFactory.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import java.util.List;
2121

2222
import static java.lang.String.format;
23-
import static java.util.Objects.requireNonNull;
2423

2524
@Singleton
2625
public class JdbcPropertiesProviderFactory
@@ -30,13 +29,13 @@ public class JdbcPropertiesProviderFactory
3029
@Inject
3130
public JdbcPropertiesProviderFactory(List<JdbcPropertiesProvider> orderedProviders)
3231
{
33-
this.providers = requireNonNull(orderedProviders, "providers is null");
32+
this.providers = List.copyOf(orderedProviders);
3433
}
3534

3635
public JdbcPropertiesProvider forConfig(DataStoreConfiguration config)
3736
{
3837
return providers.stream()
39-
.filter(p -> p.supports(config))
38+
.filter(provider -> provider.supports(config))
4039
.findFirst()
4140
.orElseThrow(() -> new IllegalArgumentException(
4241
format("No JdbcPropertiesProvider for driver: %s", config.getDriver())));

0 commit comments

Comments
 (0)