-
Notifications
You must be signed in to change notification settings - Fork 332
JDBC: Fix Bootstrap with schema options #2762
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.
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
Changes from all commits
119fbdb
6a53484
7b9054a
299868a
0f9f66a
0af7916
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -748,14 +748,28 @@ static int loadSchemaVersion( | |
| } | ||
| return schemaVersion.getFirst().getValue(); | ||
| } catch (SQLException e) { | ||
| LOGGER.error("Failed to load schema version due to {}", e.getMessage(), e); | ||
| if (fallbackOnDoesNotExist && datasourceOperations.isRelationDoesNotExist(e)) { | ||
| return SchemaVersion.MINIMUM.getValue(); | ||
| } | ||
| LOGGER.error("Failed to load schema version due to {}", e.getMessage(), e); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Do we need this error message given that we throw right after it? |
||
| throw new IllegalStateException("Failed to retrieve schema version", e); | ||
| } | ||
| } | ||
|
|
||
| static boolean entityTableExists(DatasourceOperations datasourceOperations) { | ||
| PreparedQuery query = QueryGenerator.generateEntityTableExistQuery(); | ||
| try { | ||
| List<PolarisBaseEntity> entities = | ||
| datasourceOperations.executeSelect(query, new ModelEntity()); | ||
| return entities != null && !entities.isEmpty(); | ||
| } catch (SQLException e) { | ||
| if (datasourceOperations.isRelationDoesNotExist(e)) { | ||
| return false; | ||
| } | ||
| throw new IllegalStateException("Failed to check if Entities table exists", e); | ||
| } | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override | ||
| public <T extends PolarisEntity & LocationBasedEntity> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.persistence.relational.jdbc; | ||
|
|
||
| import java.util.Optional; | ||
| import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions; | ||
| import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; | ||
|
|
||
| public class JdbcBootstrapUtils { | ||
|
|
||
| private JdbcBootstrapUtils() {} | ||
|
|
||
| /** | ||
| * Determines the correct schema version to use for bootstrapping a realm. | ||
| * | ||
| * @param currentSchemaVersion The current version of the database schema. | ||
| * @param requiredSchemaVersion The requested schema version (-1 for auto-detection). | ||
| * @param hasAlreadyBootstrappedRealms Flag indicating if any realms already exist. | ||
| * @return The calculated bootstrap schema version. | ||
| * @throws IllegalStateException if the combination of parameters represents an invalid state. | ||
| */ | ||
| public static int getRealmBootstrapSchemaVersion( | ||
| int currentSchemaVersion, int requiredSchemaVersion, boolean hasAlreadyBootstrappedRealms) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, the feedback is to infer
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more about overall confusion 😅 We're trying to figure out what version of the DDL script to run when bootstrap is called. We check the version table and the existence of some other table and the bootstrap options. However, from my POV, the big question is whether to run the DDL at all. If tables exist and already contain realm I'd think we should deduce the current schema version (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is because schema table is not realm specific :( meaning if i had a realm in version 1 and then i bootstrap with version 2 then i set the schema version globally to 2
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to have
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider my previous comments non-blocking |
||
|
|
||
| // If versions already match, no change is needed. | ||
| if (currentSchemaVersion == requiredSchemaVersion) { | ||
| return requiredSchemaVersion; | ||
| } | ||
|
|
||
| // Handle fresh installations where no schema version is recorded (version 0). | ||
| if (currentSchemaVersion == 0) { | ||
| if (hasAlreadyBootstrappedRealms) { | ||
| // System was bootstrapped with v1 before schema versioning was introduced. | ||
| if (requiredSchemaVersion == -1 || requiredSchemaVersion == 1) { | ||
| return 1; | ||
| } | ||
| } else { | ||
| // A truly fresh start. Default to v3 for auto-detection, otherwise use the specified | ||
| // version. | ||
| return requiredSchemaVersion == -1 ? 3 : requiredSchemaVersion; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM, we could improve it by having a variable like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack, let me add an ENUM in a follow-up pr |
||
| } | ||
| } | ||
|
|
||
| // Handle auto-detection on an existing installation (current version > 0). | ||
| if (requiredSchemaVersion == -1) { | ||
| // Use the current version if realms already exist; otherwise, use v3 for the new realm. | ||
| return hasAlreadyBootstrappedRealms ? currentSchemaVersion : 3; | ||
| } | ||
|
|
||
| // Any other combination is an unhandled or invalid migration path. | ||
| throw new IllegalStateException( | ||
| String.format( | ||
| "Cannot determine bootstrap schema version. Current: %d, Required: %d, Bootstrapped: %b", | ||
| currentSchemaVersion, requiredSchemaVersion, hasAlreadyBootstrappedRealms)); | ||
| } | ||
|
|
||
| /** | ||
| * Extracts the requested schema version from the provided BootstrapOptions. | ||
| * | ||
| * @param bootstrapOptions: The bootstrap options containing schema information from which to | ||
| * extract the version. | ||
| * @return The requested schema version, or -1 if not specified. | ||
| */ | ||
| public static int getRequestedSchemaVersion(BootstrapOptions bootstrapOptions) { | ||
| SchemaOptions schemaOptions = bootstrapOptions.schemaOptions(); | ||
| if (schemaOptions != null) { | ||
| Optional<Integer> version = schemaOptions.schemaVersion(); | ||
| if (version.isPresent()) { | ||
| return version.get(); | ||
| } | ||
| } | ||
| return -1; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ CREATE TABLE IF NOT EXISTS version ( | |
|
|
||
| MERGE INTO version (version_key, version_value) | ||
| KEY (version_key) | ||
| VALUES ('version', 2); | ||
| VALUES ('version', 3); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spent last night chasing test failures due to this :'(
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a copy-paste error. I guess we could add a test to validate it. We could probably do that in another PR. |
||
|
|
||
| -- H2 supports COMMENT, but some modes may ignore it | ||
| COMMENT ON TABLE version IS 'the version of the JDBC schema in use'; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker: do we need to handle a different code for H2? It would be nice to do so, but it's not related to this PR.