adutra commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2244843534
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -116,6 +116,29 @@ public void executeScript(InputStream scriptInputStream)
throws SQLException {
}
}
+ public boolean checkSchemaExists(String schemaName) {
+ final boolean[] exists = {false};
+
+ try {
+ runWithinTransaction(
+ connection -> {
+ try (ResultSet schemas = connection.getMetaData().getSchemas()) {
+ while (schemas.next()) {
+ if
(schemaName.equalsIgnoreCase(schemas.getString("TABLE_SCHEM"))) {
Review Comment:
```suggestion
if
(schemaName.equalsIgnoreCase(schemas.getString("TABLE_SCHEMA"))) {
```
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -143,15 +146,16 @@ public synchronized Map<String, PrincipalSecretsResult>
bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
- try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(bootstrapOptions.schemaOptions()));
- } catch (SQLException e) {
- throw new RuntimeException(
- String.format("Error executing sql script: %s", e.getMessage()),
e);
+ if (bootstrapOptions.schemaOptions().setupSchema()) {
+ setupSchema(bootstrapOptions.schemaOptions());
Review Comment:
We are doing this setup per realm, but the schema cannot change from one
realm to another. I think this should be done in a startup method. This would
be beneficial to both the admin tool and the server imho.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -31,6 +31,11 @@ public interface SchemaOptions {
@Nullable
String schemaFile();
+ @Value.Default
+ default boolean setupSchema() {
Review Comment:
I fell like this should go into application.properties instead. We are
creating a second layer of configuration that is only accessible to the admin
tool (I don't see how the server runtime can change this value).
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -80,6 +80,12 @@ static class FileInputOptions {
}
static class SchemaInputOptions {
+ @CommandLine.Option(
Review Comment:
The schema init scripts _must_ be idempotent, because the server runtime
executes them whenever it loads a new realm. So why do we need to specify this
here? Couldn't we just assume that schema should be set up always? If we want
to avoid re-running init scripts on an already-initialized database, this is a
concern that affects both the admin tool and the server. IOW this should be
controlled by a configuration option in application.properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
