Repository navigation
[hive] Clean up done markers in Hive partition deletion - #10964
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
Separating synthetic marker cleanup from data truncation has clear end-to-end value. On ee50ad6, the normal JDK 8 reactor package passes all 161 targeted tests (PartitionExpireTest, CachingCatalogTest, DelegateCatalogTest and HiveCatalogTest), with quality checks enabled; upstream CI is also green. An additional actual-table branch case still reproduces the exception this PR intends to fix, as described below. Temporary reviewer changes were removed.
Verified fixed at 08b5cfb. Requirement fit: SUPPORTED; implementation: CLEAN on this reviewed head. All 168 targeted JDK 8 core/Hive tests pass with normal quality checks, including empty-branch INT expiration, shared-branch marker retention and real STRING .done partitions. The original P2 is resolved; withdrawing this blocking review. The new head's CI is still running, so this is not an approval or merge.
|
Could we keep The cleanup needs to account for the shared HMS namespace: retain a marker if any branch still owns its underlying ordinary partition, or if any branch, including the current branch, contains a real partition whose literal spec equals the marker spec. These checks should use physical branch partitions without fallback reads and compare rendered partition specs, rather than converting synthetic values such as |
@JingsongLi done |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
The previous synthetic INT/branch failure and public-API concern are addressed in this revision. Normal JDK 8 checks and all 189 targeted Hive/catalog/normal-TTL/chain-TTL tests pass. An additional real-table regression found one current-branch ownership error below. Its latest-data assertion passes before the HMS registration assertion fails; temporary reviewer source changes were removed.
Purpose
Partition expiration on a Hive-backed table can commit the deletion of an ordinary INT partition and then fail when the synthetic
20230101.donemarker is passed to typed partition deletion or branch scans. Both normal and chain-table expiration are affected.Keep marker cleanup inside
HiveCatalog.dropPartitions. After the requested data partitions are deleted, HiveCatalog derives the corresponding markers from those original specs, appending.doneto the last partition key in schema order, and removes only their HMS entries. Marker cleanup does not truncate Paimon data or create another snapshot. Normal and chain-table expiration call the ordinary partition-drop path once per batch. No new public methods are added toCatalogorPartitionModification, and delegate/caching catalogs need no changes.HMS partition names are shared across branches. Preserve a marker if any branch still owns its underlying ordinary partition, or if any branch, including the current branch, owns a real partition with the marker's literal spec. These checks compare rendered partition specs from the latest physical branch metadata using each branch's schema, without fallback reads or parsing synthetic values. Ownership scans use the storage scan directly, independently of persisted query startup options such as snapshot IDs or tags; query settings remain unchanged. This enumerates broader partition metadata than a typed predicate; ordinary deletion retains its existing filtered branch check.
Marker identity comes from the original deletion request. An explicit DROP of a real STRING partition ending in
.donestill deletes its data, retains the usual branch protection for its HMS entry, and cleans up its own.done.donemarker when no branch needs it.Tests
.donepartition, and ordinary deletion with a real colliding partition on the current branch..donedata and its own marker, including shared branch ownership..done.Local validation (JDK 17), with quality checks enabled:
The extra classpath entry supplies the Hadoop client runtime corresponding to the client API on the local test classpath; no dependency changes are included.
All 195 tests in the five targeted classes pass with no skips. Standard packaging, Spotless, Checkstyle, and
git diff --checkpass.