yuqi1129 opened a new pull request, #13484:
URL: https://github.com/apache/gravitino/pull/13484
### What changes were proposed in this pull request?
`fileset_meta` and `policy_meta` get an additive `occ_version` column. The
CAS predicate and the unconditional increment move to it, and `current_version`
goes back to meaning "the history version this row points at", advancing only
when the fields `*_version_info` stores actually change.
- **Schema:** the column in `schema-2.0.0-{mysql,postgresql,h2}.sql` and in
the still-open `upgrade-1.3.0-to-2.0.0-*.sql`. `DEFAULT 1` is the whole
backfill, because `occ_version` is only ever compared against itself on the
same row: an upgraded row at `occ_version = 1` with `current_version = 7` is
correct.
- **Write path:** `updateFilesetPOWithVersion` and
`buildNextPolicyPOVersion` always advance `occ_version`; when nothing stored
changed they keep `current_version` / `last_version`, return no snapshot, and
the services skip the version insert.
- **SQL:** the CAS moves to `occ_version` in `updateFilesetMeta`,
`updatePolicyMeta`, `softDeleteFilesetMetasByFilesetId` and
`softDeletePolicyByIdAndVersion`. The fileset upsert advances `occ_version`
instead of `current_version`.
- **Read path: no query changes.** `current_version` moves only in the
statement that inserts the snapshot it moves to, so a live row still points at
exactly one active snapshot.
One subtlety worth pointing at during review: `updateFilesetMeta`'s `NOT
EXISTS` snapshot check is appended **only** when the alter allocates a version.
An alter that allocates none keeps `current_version` where it is, and the
snapshot it points at is supposed to exist, so the check would otherwise reject
every such alter.
### Why are the changes needed?
`current_version` was doing two jobs at once: it is the join key into
`*_version_info`, and it was the value the CAS compared. Once OCC made the
token advance on every alter (#12656, #12782), an audit-only or rename-only
alter had to write a full snapshot just to keep the join resolvable — for a
fileset that is one row per storage location — and the retention job removed
them again. The history table recorded revisions that revised nothing.
Fix: #12206
**Scope note:** #12206's body is largely stale against `main`. The full-row
comparisons it describes are gone (#12656 for fileset, #12782 for policy), and
the `model_meta` insert it calls out already lists `current_version` /
`last_version` (`ModelMetaBaseSQLProvider.java:36`, `:49`). What remained is
the coupling named in the title. `table` / `view` / `function` share it and are
left to a follow-up: their `current_version` has many more callers.
### Does this PR introduce _any_ user-facing change?
No. Neither fileset nor policy version history is reachable through any
public API — `FilesetVersionMapper` and `PolicyVersionMapper` are used only by
the `current_version` join and the retention job, and `grep
getCurrentVersion()` outside `storage/relational` has no hits, so nothing
depends on it increasing monotonically. No API, configuration or read-path
change.
One benign behavioural consequence: because metadata-only alters no longer
consume version numbers, retention evicts genuine content history more slowly
than before.
### How was this patch tested?
`TestFilesetMetaService` 60, `TestPolicyMetaService` 60, `TestPOConverters`
41, `TestFilesetMetaBaseSQLProvider` 5, `TestFilesetMetaPostgreSQLProvider` 2 —
168 tests, 0 failures. `-Werror` compile clean.
New tests:
- `testAlterWritesASnapshotOnlyWhenStoredContentChanges` — a fileset with
two storage locations: a rename leaves `fileset_version_info` at 2 rows and 1
distinct version while `occ_version` advances; a comment change takes it to 4
rows and 2 versions; the fileset still reads back correctly. Added a
`countFilesetVersionRows` helper, because the row count rather than the version
count is what a snapshot costs.
- `testDeleteRejectsAStaleVersionAfterAMetadataOnlyAlter` — pins the
regression this decoupling creates. An audit-only alter no longer moves
`current_version`, so a drop still guarded by it would stop detecting one and
would delete a fileset the caller never observed in its current state.
Mutation-checked: with the delete CAS on `current_version` it fails with
`Expected OptimisticLockException to be thrown, but nothing was thrown`.
- `testUpdateDropsTheSnapshotCheckWhenNoVersionIsAllocated` — pins the
conditional `NOT EXISTS`.
The `filesetSnapshotUnchanged` branch is mutation-checked too: disabling it
makes `testUpdateFilesetPOVersion` fail (`expected: <1> but was: <2>`).
Two existing tests were rewritten because they asserted the behaviour this
PR reverses: `testMetadataOnlyPolicyAlterCreatesCompleteSnapshot` →
`...AdvancesOnlyTheOccVersion`, and the tail of
`testAlterReportsOptimisticLockConflictAndKeepsWinnerVersion`.
**Not covered — evidence gaps, stated rather than implied:**
- MySQL and PostgreSQL are exercised through their dialect SQL against H2,
not against real engines. `occ_version INT UNSIGNED` (MySQL/H2) vs `INT`
(PostgreSQL), and PostgreSQL's `ON CONFLICT ... DO UPDATE SET occ_version =
fileset_meta.occ_version + 1`, have only SQL-text assertions behind them.
`-PdockerTest=true` or CI is needed.
- The legacy `maxStoredVersion` retry has converter-level coverage only
(`TestPOConverters`), no DB-level test. That was already the case before this
PR.
- `insertFilesetMetaOnDuplicateKeyUpdate` has no production caller (the
fileset overwrite path goes through `updateFilesetMeta`), so its change is
covered by SQL-text tests only.
--
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]
