zhuxiangyi commented on PR #10098:
URL: https://github.com/apache/paimon/pull/10098#issuecomment-5817270950
@JingsongLi thanks for the review. All four points were real. I reproduced
each one with a deterministic test before changing anything, and they are fixed
in new commits on top of the branch. Akash's three inline threads are answered
individually with the code references.
**1. In-flight stale writer (P1), 37ff2bdfb**
After the schema change, the procedure now commits a fence: an empty
snapshot through the normal commit path. It then assigns row ids to anything
that was committed before the fence.
A writer reads its base snapshot before it checks the schema. So a writer
that passed the check on the old schema has only two outcomes:
- it committed before the fence, and the repair after the fence assigns ids
to its files;
- its snapshot CAS loses to the fence, and its retry is refused by the
schema check.
Tests pause a real writer right after its schema check, with a `FileIO` that
blocks its first manifest write, and release it at three points:
- after the procedure returned: refused, and no file without row id is left;
- between the schema change and the fence: committed, then repaired;
- on an empty table, where the fence is the first snapshot: refused.
**2. Rollback guards on cached options (P1), 35110ec00**
`rollbackTo(snapshot)`, `rollbackTo(tag)` and `rollbackToAsLatest` now
decide by the latest persisted schema. Tested through a table object that was
loaded before the conversion.
**3. Stale schema id on the row id snapshot (P2), 37ff2bdfb**
Every row id commit attempt reads the latest schema right before the
replacement.
**4. Direct schema change (P1), 7eb1237cc and 37ff2bdfb**
- The action is out of the REST protocol, both the JSON subtypes and the
OpenAPI spec, and `RESTCatalog.alterTable` rejects it.
- For the other catalogs, the schema manager accepts it only on a table
without snapshot, or when the latest snapshot is a row id commit of the
procedure. Such a commit is marked with its own snapshot id, so a copied mark
does not count.
- If a writer commits between the row id commit and the schema change, the
procedure assigns ids again and retries, bounded by the commit retry settings.
**Older writer binaries:** agreed, this cannot be enforced in code. The
conversion section of the docs now requires stopping writers of older Paimon
versions before converting.
**One more bug, found while checking the tests (not raised in the review),
ff3eeba71**
A plain append writer numbers its rows, so a file written in one commit of
many rows has sequence numbers far above the snapshot ids of later commits. A
data-evolution read takes each column from the file with the highest sequence
number. After the conversion, a partial-column write or a `MERGE INTO` on such
a file was therefore silently ignored; the small test tables hid it. Converted
files now get the sequence number of the row id commit, like any row-tracking
commit. New tests write a 20-row commit (core) and a 100-row commit (Spark
`UPDATE` and `MERGE INTO`); both fail without the fix.
**Also**
- A run that finds the schema already switched by a concurrent run does not
switch it again.
- 79606fad8 covers expiring the pre-conversion snapshots and deleting a
pre-conversion tag: the data files that the conversion re-references through
new manifests are kept.
- A conversion now commits two snapshots: the OVERWRITE snapshot that
assigns the row ids, and the empty APPEND fence.
**Verification**
- Every fix was checked by mutation: reverting it makes at least one of the
new tests fail.
- Locally on the final head:
- 29 core suites (1006 tests: all `DataEvolution*`, `RowTracking*` and
rollback suites, `SchemaManagerTest`, `MockRESTCatalogTest` and more);
- `EnableDataEvolutionProcedureITCase` (Flink);
- `EnableDataEvolutionProcedureTest` and `RowTrackingTest` (Spark 3.5).
- CI is running for the other Spark versions.
--
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]
