flyrain commented on code in PR #2762:
URL: https://github.com/apache/polaris/pull/2762#discussion_r2408061186
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -396,7 +400,8 @@ public boolean isConstraintViolation(SQLException e) {
}
public boolean isRelationDoesNotExist(SQLException e) {
- return RELATION_DOES_NOT_EXIST.equals(e.getSQLState());
+ return RELATION_DOES_NOT_EXIST.equals(e.getSQLState())
+ || H2_RELATION_DOES_NOT_EXIST.equals(e.getSQLState());
Review Comment:
Is it possible that H2 status codes collide with postgres ones? In that
case, it may misinterpret the status code. I believe we may need introduce a
type somewhere, which is also needed for `mysql` and `CockRoach DB`.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -33,7 +34,8 @@ public interface SchemaOptions {
@Value.Check
default void validate() {
- if (schemaVersion() != null && schemaFile() != null) {
+ if (schemaVersion() != null
+ && (schemaFile() != null &&
!Objects.requireNonNull(schemaFile()).isEmpty())) {
Review Comment:
Can we use something like `Strings.isNullOrEmpty()` from Guava to simplify a
bit?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java:
##########
@@ -53,9 +54,10 @@ public static DatabaseType fromDisplayName(String
displayName) {
* caller.
*/
public InputStream openInitScriptResource(@Nonnull SchemaOptions
schemaOptions) {
- if (schemaOptions.schemaFile() != null) {
+ if (schemaOptions.schemaFile() != null
+ && !Objects.requireNonNull(schemaOptions.schemaFile()).isEmpty()) {
Review Comment:
--> `Strings.isNullOrEmpty()`?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -281,4 +299,23 @@ private void
checkPolarisServiceBootstrappedForRealm(RealmContext realmContext)
"Realm is not bootstrapped, please run server in bootstrap mode.");
}
}
+
+ private int getSchemaVersion(BootstrapOptions bootstrapOptions) {
Review Comment:
Combining with the comment
here(https://github.com/apache/polaris/pull/2762/files#r2408431752), maybe we
could have a method taking both current schema and request schema, then emit
the final decision.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -154,6 +159,19 @@ public synchronized Map<String, PrincipalSecretsResult>
bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
+ int schemaVersion =
+ JdbcBasePersistenceImpl.loadSchemaVersion(
+ datasourceOperations,
+ configurationStore.getConfiguration(
+ realmContext,
BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
+ // skip validation if no schema is specified.
Review Comment:
Should we try-catch line 162 - 166, as it may throw? What the expected
behavior when it throws? Skipping the version validation or error out?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -154,6 +159,19 @@ public synchronized Map<String, PrincipalSecretsResult>
bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
+ int schemaVersion =
+ JdbcBasePersistenceImpl.loadSchemaVersion(
+ datasourceOperations,
+ configurationStore.getConfiguration(
+ realmContext,
BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
+ // skip validation if no schema is specified.
+ int requestedSchemaVersion = getSchemaVersion(bootstrapOptions);
+ Preconditions.checkState(
+ (requestedSchemaVersion == schemaVersion)
+ || (schemaVersion == 0 || requestedSchemaVersion == -1),
Review Comment:
The version we want to bootstrapped should be `schemaVersion` when
requestedSchemaVersion is -1. The current logic will always go to `v3`. Would
this be problematic?
```
switch (schemaOptions.schemaVersion()) {
case null -> schemaSuffix = "schema-v3.sql";
case 1 -> schemaSuffix = "schema-v1.sql";
case 2 -> schemaSuffix = "schema-v2.sql";
case 3 -> schemaSuffix = "schema-v3.sql";
```
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -281,4 +299,23 @@ private void
checkPolarisServiceBootstrappedForRealm(RealmContext realmContext)
"Realm is not bootstrapped, please run server in bootstrap mode.");
}
}
+
+ private int getSchemaVersion(BootstrapOptions bootstrapOptions) {
Review Comment:
Would you mind adding a test for this? I think it's important to prevent
regression. I assume the test would take a list of combination of current
schema and request schema, and check the result based on the inputs like the
following:
1. 0, -1 -> 1
2. 1, -1 -> 1
3. 0, 1 -> 1
4. equals -> the same version
5. 2, 3 -> mismatch, error out
6. No realm, -1 -> 3
7. No realm, 2 -> 2
...
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -154,6 +159,19 @@ public synchronized Map<String, PrincipalSecretsResult>
bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
+ int schemaVersion =
+ JdbcBasePersistenceImpl.loadSchemaVersion(
+ datasourceOperations,
+ configurationStore.getConfiguration(
+ realmContext,
BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
+ // skip validation if no schema is specified.
+ int requestedSchemaVersion = getSchemaVersion(bootstrapOptions);
+ Preconditions.checkState(
+ (requestedSchemaVersion == schemaVersion)
+ || (schemaVersion == 0 || requestedSchemaVersion == -1),
Review Comment:
In case schemaVersion is 0, I think we have to make sure
requestedSchemaVersion is 1.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
