Skip to content
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

Simplify config loading #102

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Simplify config loading #102

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
17 changes: 11 additions & 6 deletions hoptimator-jdbc/src/main/java/com/linkedin/hoptimator/jdbc/HoptimatorDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.Properties;
import java.util.function.Supplier;

import org.apache.calcite.avatica.ConnectStringParser;
import org.apache.calcite.avatica.DriverVersion;
import org.apache.calcite.jdbc.CalciteConnection;
import org.apache.calcite.jdbc.CalcitePrepare;
Expand Down Expand Up @@ -69,28 +70,32 @@ public Connection connect(String url, Properties props) throws SQLException {
CalciteConnection calciteConnection = (CalciteConnection) connection;
SchemaPlus rootSchema = calciteConnection.getRootSchema();

String remaining = url.substring(getConnectStringPrefix().length()).trim();
String[] catalogs = remaining.split(",");

// built-in schemas
rootSchema.add("DEFAULT", new AbstractSchema());

calciteConnection.setSchema("DEFAULT");

WrappedSchemaPlus wrappedRootSchema = new WrappedSchemaPlus(rootSchema);

// Load properties from url and from getConnection()
Properties properties = new Properties();
properties.putAll(props); // via getConnection()
properties.putAll(ConnectStringParser.parse(url.substring(getConnectStringPrefix().length())));
String[] catalogs = properties.getProperty("catalogs", "").split(",");

if (catalogs.length == 0 || catalogs[0].length() == 0) {
// load all catalogs (typical usage)
for (Catalog catalog : CatalogService.catalogs()) {
catalog.register(wrappedRootSchema, props);
catalog.register(wrappedRootSchema, properties);
}
} else {
// load specific catalogs when loaded as `jdbc:hoptimator://foo,bar`
// load specific catalogs when loaded as `jdbc:hoptimator://catalogs=foo,bar`
for (String catalog : catalogs) {
CatalogService.catalog(catalog).register(wrappedRootSchema, props);
}
}

return new HoptimatorConnection(calciteConnection, props);
return new HoptimatorConnection(calciteConnection, properties);
} catch (Exception e) {
throw new SQLException("Problem loading " + url, e);
}
Expand Down
2 changes: 1 addition & 1 deletion hoptimator-jdbc/src/testFixtures/java/com/linkedin/hoptimator/jdbc/QuidemTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ protected void run(URI resource) throws IOException {
Quidem.Config config = Quidem.configBuilder()
.withReader(r)
.withWriter(w)
.withConnectionFactory((x, y) -> DriverManager.getConnection("jdbc:hoptimator://" + x))
.withConnectionFactory((x, y) -> DriverManager.getConnection("jdbc:hoptimator://catalogs=" + x))
.withCommandHandler(new CustomCommandHandler())
.build();
new Quidem(config).execute();
Expand Down
6 changes: 2 additions & 4 deletions hoptimator-kafka/src/main/java/com/linkedin/hoptimator/kafka/KafkaDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
import org.apache.calcite.jdbc.Driver;
import org.apache.calcite.schema.SchemaPlus;

import com.linkedin.hoptimator.util.ConfigService;


/** JDBC driver for Kafka topics. */
public class KafkaDriver extends Driver {
Expand All @@ -36,9 +34,9 @@ public Connection connect(String url, Properties props) throws SQLException {
if (!url.startsWith(getConnectStringPrefix())) {
return null;
}
// Connection string properties are given precedence over config properties
Properties properties = ConfigService.config(null);
Properties properties = new Properties();
properties.putAll(ConnectStringParser.parse(url.substring(getConnectStringPrefix().length())));
properties.putAll(props); // in case the driver is loaded via getConnection()
Comment on lines 38 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these lines be reversed? Trying to think if there are config collisions, which should take precedence. I'd think the connection properties should (same as you have for HoptimatorDriver)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The JDBC spec says the precedence order is undefined and driver specific! So whatever makes sense to us I guess. Agree that the URL should take precedence, since that is more likely to be what end users are actually able to manipulate. Will fix, thx.

try {
Connection connection = super.connect(url, props);
if (connection == null) {
Expand Down
1 change: 0 additions & 1 deletion hoptimator-util/src/main/java/com/linkedin/hoptimator/util/ConfigService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ public final class ConfigService {
private ConfigService() {
}

// Null namespace will default to current namespace, may not be used by some ConfigProviders.
// loadTopLevelConfigs=true loads top level configs and expands input fields as file-like properties
// loadTopLevelConfigs=false will only expand input fields as file-like properties
// Ex:
Expand Down
5 changes: 2 additions & 3 deletions hoptimator-venice/src/main/java/com/linkedin/hoptimator/venice/VeniceDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
import org.apache.calcite.jdbc.Driver;
import org.apache.calcite.schema.SchemaPlus;

import com.linkedin.hoptimator.util.ConfigService;


/** JDBC driver for Venice stores. */
public class VeniceDriver extends Driver {
Expand All @@ -40,8 +38,9 @@ public Connection connect(String url, Properties props) throws SQLException {
return null;
}
// Connection string properties are given precedence over config properties
Properties properties = ConfigService.config(null, CONFIG_NAME);
Properties properties = new Properties();
properties.putAll(ConnectStringParser.parse(url.substring(getConnectStringPrefix().length())));
properties.putAll(props); // in case the driver is loaded via getConnection()
Comment on lines 42 to +43
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

String cluster = properties.getProperty("cluster");
if (cluster == null) {
throw new IllegalArgumentException("Missing required cluster property. Need: jdbc:venice://cluster=...");
Expand Down