yuqi1129 commented on code in PR #13689:
URL: https://github.com/apache/gravitino/pull/13689#discussion_r4215597826
##########
scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:
##########
@@ -121,14 +468,21 @@ ALTER TABLE `function_version_info` RENAME INDEX
`idx_sid` TO `function_version_
-- SELECT COUNT(*) FROM `table_version_info`
-- WHERE `version` IS NULL OR `deleted_at` IS NULL;
-- The statement below fails rather than silently coercing NULL to 0.
-ALTER TABLE `table_version_info`
- MODIFY COLUMN `version` BIGINT(20) UNSIGNED NOT NULL COMMENT 'table
current version',
- MODIFY COLUMN `deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT
'table deletion timestamp, 0 means not deleted',
- DROP INDEX `uk_table_id_version_deleted_at`,
- ADD PRIMARY KEY (`table_id`, `version`, `deleted_at`);
+SET @ddl = IF(EXISTS(SELECT 1 FROM information_schema.statistics
+ WHERE table_schema = DATABASE() AND table_name = 'table_version_info' AND
index_name = 'PRIMARY'),
+ 'SELECT 1',
+ 'ALTER TABLE `table_version_info` MODIFY COLUMN `version` BIGINT(20)
UNSIGNED NOT NULL COMMENT ''table current version'', MODIFY COLUMN `deleted_at`
BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT ''table deletion timestamp, 0
means not deleted'', DROP INDEX `uk_table_id_version_deleted_at`, ADD PRIMARY
KEY (`table_id`, `version`, `deleted_at`)');
Review Comment:
Fixed in e6d4a0a. The guard now skips only when PRIMARY has exactly
(table_id, version, deleted_at) in that order and
uk_table_id_version_deleted_at is absent. Added regressions for an unrelated
primary key, reordered columns with the old index removed, and the target key
with the old unique index still present. The final MySQL regression run passed;
unexpected states still fail the ALTER.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,219 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes, readSchemaIndexes());
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertEquals(1176, ((SQLException)
failure.getCause()).getErrorCode());
+ }
+ }
+
+ /** Verifies retries after structural DDL and data updates, including
prepared DDL execution. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroRetriesPartialExecution() throws
SQLException, IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File schema = scriptDir.resolve("schema-1.3.0" + suffix).toFile();
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ List<String> statements = extractStatements(upgrade.toPath());
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ executeScript(upgrade);
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+
+ for (int completed = 1; completed <= statements.size(); completed++) {
+ String last = statements.get(completed - 1).toUpperCase();
+ if (!(last.startsWith("ALTER TABLE")
+ || last.startsWith("CREATE ")
+ || last.startsWith("UPDATE ")
+ || last.startsWith("EXECUTE "))) {
+ continue;
+ }
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ // Close the connection after the prefix, as if the migration process
stopped here.
+ String checkpoint = "Interrupted after statement " + completed + ": " +
last;
+ executeStatements(statements.subList(0, completed), checkpoint);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns(),
checkpoint);
+ Assertions.assertEquals(expectedIndexes, readSchemaIndexes(),
checkpoint);
+ Assertions.assertEquals(expectedData, readUpgradeData(), checkpoint);
+ if ("mysql".equals(backendType)) {
+ try (SqlSession session =
+
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
+ Connection connection = session.getConnection();
+ ResultSet keys =
+ connection
+ .getMetaData()
+ .getPrimaryKeys(connection.getCatalog(), null,
"table_version_info")) {
+ List<String> columns = new ArrayList<>();
+ while (keys.next()) {
+ columns.add(keys.getString("COLUMN_NAME"));
+ }
+ Assertions.assertEquals(
+ Set.of("table_id", "version", "deleted_at"),
Set.copyOf(columns), checkpoint);
+ }
+ }
+ }
+ }
+
+ private Path upgradeScriptDirectory() {
+ String home = System.getenv("GRAVITINO_HOME");
+ Assertions.assertNotNull(home, "GRAVITINO_HOME environment variable is not
set");
+ return Path.of(home, "scripts", backendType.toLowerCase());
+ }
+
+ private void seedUpgradeData() throws SQLException {
+ executeStatements(
+ List.of(
+ "INSERT INTO idp_user_meta (user_id, user_name, password_hash,
current_version, last_version) VALUES (1, 'user', 'hash', 7, 9)",
+ "INSERT INTO idp_group_meta (group_id, group_name) VALUES (1,
'group')",
+ "INSERT INTO tag_meta (tag_id, tag_name, metalake_id, audit_info)
VALUES (1, 'tag', 1, '{}')",
+ "INSERT INTO tag_relation_meta (id, tag_id, metadata_object_id,
metadata_object_type, audit_info) VALUES (1, 1, 1, 'TABLE', '{}')",
+ "INSERT INTO fileset_meta (fileset_id, fileset_name, metalake_id,
catalog_id, schema_id, type, audit_info, current_version, last_version) VALUES
(1, 'fileset', 1, 1, 1, 'MANAGED', '{}', 7, 9)",
+ "INSERT INTO policy_meta (policy_id, policy_name, policy_type,
metalake_id, audit_info, current_version, last_version) VALUES (1, 'policy',
'custom', 1, '{}', 7, 9)",
+ "INSERT INTO table_version_info (table_id, version, deleted_at)
VALUES (1, 7, 0)"),
Review Comment:
Fixed in e6d4a0a: split the long seed-data SQL literals into shorter
concatenated strings. Core Spotless checks passed.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,219 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes, readSchemaIndexes());
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
Review Comment:
Fixed in e6d4a0a. Both new retry tests now compare the upgraded columns and
indexes against a separately loaded schema-2.0.0 for the same backend. The only
excluded table is the deliberately retained policy_relation_meta.
MySQL/PostgreSQL index names are compared; H2 compares index definitions and
multiplicity because its generated constraint-index names vary. These
comparisons passed on all three backends.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,219 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes, readSchemaIndexes());
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertEquals(1176, ((SQLException)
failure.getCause()).getErrorCode());
+ }
+ }
+
+ /** Verifies retries after structural DDL and data updates, including
prepared DDL execution. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroRetriesPartialExecution() throws
SQLException, IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File schema = scriptDir.resolve("schema-1.3.0" + suffix).toFile();
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ List<String> statements = extractStatements(upgrade.toPath());
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ executeScript(upgrade);
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+
+ for (int completed = 1; completed <= statements.size(); completed++) {
Review Comment:
Fixed in e6d4a0a. readSchemaIndexes now takes the already-computed table
set, avoiding the extra readSchemaColumns call. The interruption matrix samples
the first and last MySQL RENAME INDEX checkpoints and retains all other
structural/data-update checkpoints: 30 MySQL, 27 PostgreSQL, and 21 H2 (78
total). The final matrix passed on all three backends.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,219 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes();
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes, readSchemaIndexes());
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertEquals(1176, ((SQLException)
failure.getCause()).getErrorCode());
Review Comment:
Fixed in e6d4a0a. The assertion now checks that the SQLException message
names idx_mid instead of requiring error code 1176. The MySQL idempotency test
passed.
##########
scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:
##########
@@ -17,30 +17,64 @@
-- under the License.
--
+-- MySQL has no ADD COLUMN / CREATE INDEX IF NOT EXISTS. Guard completed DDL
+-- so a failed upgrade can be restarted from the beginning without --force.
+-- Keep each operation in this session; no stored routines or extra privileges
are required.
Review Comment:
Fixed in e6d4a0a. Added a note in Step 5 of docs/how-to-upgrade.md
explaining that the MySQL/PostgreSQL/H2 1.3.0-to-2.0.0 scripts can be rerun
after fixing the failure cause, earlier scripts are unchanged, and unexpected
schema differences require manual repair before retrying.
--
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]
