jerryshao commented on code in PR #13689:
URL: https://github.com/apache/gravitino/pull/13689#discussion_r4215337024
##########
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:
[Important] Both new tests snapshot `expectedColumns`/`expectedIndexes` from
an already-successful upgrade and then compare the retried database against
that same snapshot. That catches a retry which *diverges*, but never a guard
that is wrong on the first run: an inverted guard condition, or an `IF EXISTS`
drop that silently no-ops because the object has a different name, yields
identical expected and actual values and passes. For H2 it is weaker still,
because `readSchemaIndexes` (line 445) blanks index names, so only structure is
compared.
The helpers you just added make the missing assertion cheap: drop the
database, load `schema-2.0.0` + suffix, and compare
`readSchemaColumns()`/`readSchemaIndexes()` with the values captured from the
upgraded database. I ran exactly that comparison for H2 2.2.224 in `MODE=MYSQL`
with the upgrade applied three times - it passes, with `policy_relation_meta`
the only difference, which the script header at
`scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:24-25` documents as
intentional. One assertion with that single documented exception would pin the
property the script's own comment claims at
`scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:132-136` ("A database upgraded
from 1.3.0 ends up with the same index names as a fresh 2.0.0 install"), which
nothing in the suite checks today.
Verified by: ran `schema-1.3.0-h2.sql` plus three passes of
`upgrade-1.3.0-to-2.0.0-h2.sql` against a fresh `schema-2.0.0-h2.sql` on H2
2.2.224 over JDBC, diffing `getColumns`/`getIndexInfo`; read both new tests and
all helpers in full.
##########
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:
[Nit] This loop rebuilds the whole fixture once per structural statement: 59
iterations for MySQL, 27 for PostgreSQL, 21 for H2. Each iteration drops
roughly 60 tables, replays `schema-1.3.0`, replays all 203 statements of the
MySQL upgrade, and then reads metadata three times - and `readSchemaIndexes()`
calls `readSchemaColumns()` again (line 428) before issuing one `getIndexInfo`
per table, so every checkpoint pays about 60 extra metadata round trips on top.
Against dockerised MySQL that is a lot of wall clock for one test.
Two cheap reductions: pass the already-computed table set into
`readSchemaIndexes` instead of re-reading all columns, and sample the 31
near-identical `RENAME INDEX` checkpoints rather than running every one of them
- they all exercise the same guard shape.
Verified by: re-derived the per-engine checkpoint counts (59/27/21, 107
total, matching the PR description) by applying this method's prefix filter to
each upgrade script; read `readSchemaIndexes` at line 428.
##########
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:
[Nit] This comment is the only place the new contract is written down, and
`docs/how-to-upgrade.md` still tells operators the opposite. Step 5 there (line
159) says just "These scripts should run to completion without any errors. If
you do encounter errors you need to analyze the cause and attempt to trace it
back to one of the preceding steps." Nothing tells an operator that re-running
the script is now the supported recovery from a partial failure, that this
holds for 1.3.0 -> 2.0.0 only (earlier upgrade scripts are unchanged), or that
a schema-drift error such as MySQL 1176 on a rename means manual repair rather
than another retry. Given the PR lists this as a user-facing change, a two-line
note in Step 5 would make it discoverable.
Verified by: read `docs/how-to-upgrade.md` Steps 4-6; `git diff --stat
origin/main...HEAD` touches only the three scripts and `TestSQLScripts.java`,
no docs.
##########
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:
[Nit] Pinning the numeric code 1176 (`ER_KEY_DOES_NOT_EXITS`) ties the test
to one server's error numbering; MariaDB or a future MySQL release may report
the rename failure differently, and the assertion would then fail with nothing
more informative than `expected: 1176`. Asserting that the exception message
names the missing index (`idx_mid`) expresses the same intent and survives a
backend change.
Verified by: read the block above; the guard being exercised is the
`schema_meta` rename at
`scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:137-141`.
--
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]
