Make table deletion mutually exclusive with a deletion marker znode - #19254
Open
guruguha wants to merge 5 commits into
Open
Make table deletion mutually exclusive with a deletion marker znode#19254guruguha wants to merge 5 commits into
guruguha wants to merge 5 commits into
Conversation
added 3 commits
August 13, 2026 15:01
…mance
Fixed critical bug where DELETE /tables/{tableName} was not atomic, allowing
concurrent deletions to run in parallel with no mutual exclusion. This could
lead to race conditions, metadata corruption, and inconsistent cluster state.
Key changes:
- Added deletion marker znode-based distributed locking mechanism to prevent
concurrent table deletions across controllers
- Enhanced addTable() to check for deletion markers and prevent table recreation
during deletion operations
- Implemented 24-hour expiry on deletion markers to prevent permanent blocking
- Optimized SegmentDeletionManager to batch segment move operations in the
retention>0 path, reducing deletion time from hours to minutes for large tables
- Added logic to honour table config's deletedSegmentsRetentionPeriod setting
- Implemented post-deletion validation to ensure all table-related znodes are
successfully removed before marking deletion complete
- Added comprehensive unit tests for deletion marker functionality
The deletion marker approach provides mutual exclusion for table deletions while
allowing for controller failover scenarios through the expiry mechanism. The
validation step ensures data consistency by detecting partial deletions.
The previous commit did not compile, so CI never validated any of it. - ZKMetadataProvider: getLongField takes a default value and returns a primitive, so read the marker start time with a -1 sentinel instead of assigning to a Long. - PinotHelixResourceManager: HelixManager exposes getInstanceName(), not getInstanceId(). - Replace validateTableDeletionCompleteness with auditTableDeletionCompleteness. Every property store getter it called was invented and does not exist. It now uses the real APIs (InstancePartitionsUtils.fetchInstancePartitions, ZKMetadataProvider.getSegments, SegmentLineageAccessHelper) and only audits artifacts that deleteTable removes with a synchronous write. The ExternalView check is dropped because Helix removes it asynchronously, so it is routinely still present and would report a false leak. It now logs a warning rather than throwing: the table config is already gone by that point, so failing the request would report a misleading error for a table that no longer exists. - SegmentDeletionManager: only touch a destination whose move succeeded. Touching a failed destination can materialize an empty object on object stores and leave a phantom deleted segment for the retention manager. Failed moves are now reported at warn level instead of debug so they stay visible. There is no PinotFS.moveBatch, so this path is still one move per segment; it saves an exists() round trip and a log line per segment, nothing more. - Rewrite TableDeletionMarkerTest against an embedded ZkStarter server instead of a hard-coded localhost:2181, make the cases order independent, and assert marker contents plus real expiry and takeover behaviour. - Add a SegmentDeletionManagerTest regression test for the touch-on-failed-move bug, verified to fail before the fix.
…rsing Adversarial re-review of the change turned up three real defects. - createOrTakeoverTableDeletionMarker performed a non-atomic read-modify-write: exists, get, remove, create. Two controllers could both observe the same stale marker, and the second one's remove would delete the first one's freshly created marker, leaving both believing they held the lock and deleting the same table concurrently, which is the exact failure this marker exists to prevent. It also left the path briefly empty, during which addTable() would allow a re-create mid-deletion. Replaced with a version checked set, the same ZkBadVersionException idiom already used by setSegmentZKMetadata, so exactly one racing controller wins and the marker is never absent. - removeTableDeletionMarker released the marker unconditionally. A controller that stalled past the expiry window and was taken over would delete the new owner's marker on its way out. It now takes the expected owner id and is a no-op when the marker belongs to someone else. - deleteTable hand-rolled the table config retention lookup, duplicating SegmentDeletionManager.getRetentionMsFromTableConfig. The hand-rolled version guarded on null rather than emptiness and had no try/catch, so a table with an empty or malformed deletedSegmentsRetentionPeriod would throw out of TimeUtils.convertPeriodToMillis and become permanently undeletable. Now calls the existing helper: 24 lines become 2 and both cases are handled. Also moved the materialized view dependency check ahead of marker acquisition so a refused delete no longer leaves a marker that makes addTable() report a deletion in progress for a table nobody is deleting. Tests: added coverage for ownership checked release and for two controllers racing to acquire the marker (using two independent property stores, since one ZkHelixPropertyStore serializes its own operations). The single-winner test asserts the invariant but is NOT a regression test for the non-atomic takeover: that window is sub-millisecond and could not be hit reliably, and a ZK watch cannot distinguish remove-then-create from an in-place set because zkclient dispatches on the node state at event processing time. The takeover's correctness rests on the ZK version check itself.
guruguha
changed the title
A concurrent delete, or a create racing an in-flight delete, is an expected and retryable condition, not a server fault. Both guards threw IllegalStateException, which PinotTableRestletResource maps to INTERNAL_SERVER_ERROR, so a routine race surfaced as a 500 and would show up in monitoring as an outage. Both now throw ControllerApplicationException with Response.Status.CONFLICT. addTable already rethrows ControllerApplicationException untouched, so it needed no REST change. The DELETE handler wrapped every exception into a 500, so it now rethrows ControllerApplicationException first rather than downgrading a deliberate status. PinotHelixResourceManager already throws ControllerApplicationException elsewhere, so this follows the existing pattern in the class. Verified: PinotTableRestletResourceTest (27 tests, covers the DELETE handler including the 404-when-absent path), SegmentDeletionManagerTest (9), TableDeletionMarkerTest (16). Checkstyle and license clean.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19254 +/- ##
============================================
+ Coverage 66.94% 66.96% +0.02%
Complexity 1423 1423
============================================
Files 3453 3453
Lines 218858 219029 +171
Branches 34787 34825 +38
============================================
+ Hits 146512 146676 +164
- Misses 60621 60633 +12
+ Partials 11725 11720 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codecov flagged 4 uncovered lines, all in the stale-marker takeover, which is the load-bearing branch of this change: the version check is the only thing stopping two controllers from both reclaiming a stale marker. Extracted the compare-and-set step into takeoverStaleTableDeletionMarker(..., expectedVersion), marked @VisibleForTesting, so a test can supply a deliberately stale version. That is exactly the state a losing controller is in, and it makes the race deterministically testable where the real sub-millisecond interleaving could not be forced. Two tests added: a takeover with a superseded version must fail and must leave the winner's ownership intact, and a takeover with the current version wins. This also settles a factual question: on a version mismatch ZkHelixPropertyStore returns false rather than throwing ZkBadVersionException, so the false branch is the path a losing controller actually takes. The catch is kept because setSegmentZKMetadata in this same file documents Helix throwing for the same accessor type, and the comment now records both. Those 3 catch-block lines remain uncovered by design; they are unreachable for this accessor. TableDeletionMarkerTest: 16 -> 18 tests, all passing. Checkstyle and license clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
DELETE /tables/{tableName}has no mutual exclusion. The endpoint is not@LeaderOnly, so any controller can serve it, andPinotHelixResourceManager.deleteTable()is not synchronized (unlikedeleteSegment()andaddInstance()). Two concurrent deletes of the same table therefore interleave freely, and because the table config is removed last, aPOST /tablescan also re-create a table while an in-flight deletion is still tearing its metadata down — the new table then loses metadata to the older delete.Note that
synchronizedalone cannot fix this: the request can land on any controller, so the exclusion has to be cluster-wide.What this changes
Deletion marker znode (
/TABLE_DELETION_IN_PROGRESS/<tableNameWithType>)deleteTable()acquires a marker before touching any metadata and releases it in afinally. A second concurrent delete of the same table is rejected withIllegalStateException.addTable()refuses to create a table while a valid marker exists for it.propertyStore.create()(atomic) for the common case, and a version-checkedset()when reclaiming a stale marker, following the existingZkBadVersionExceptionidiom inZKMetadataProvider.setSegmentZKMetadata(). A remove-then-create reclaim would be a non-atomic read-modify-write: two controllers could both observe the same stale marker and both believe they acquired it, and it would leave the path briefly empty, which would letaddTable()through.Honour the table config's
deletedSegmentsRetentionPeriodindeleteTable()deleteTable()calledremoveSegmentsFromStoreInBatch()with only the request'sretentionquery parameter, so when that was absent the table's owndeletedSegmentsRetentionPeriodwas ignored and the cluster-wide default applied. It now falls back toSegmentDeletionManager.getRetentionMsFromTableConfig(), which is the existing helper used by thedeleteSegments(..., TableConfig)path and which tolerates an empty or malformed period rather than throwing.Post-deletion metadata audit (warn only)
After a delete completes, the property store is checked for artifacts that
deleteTable()should have removed, and anything left behind is logged atwarnfor an operator to clean up.This deliberately does not throw: by that point the table config is already gone, so the delete has succeeded from the caller's point of view, and failing the request would report a misleading error for a table that no longer exists. It also deliberately does not check the ExternalView, which Helix removes asynchronously after the IdealState drops and which is therefore routinely still present at that point. Tier instance partitions, minion task metadata and materialized view metadata are not audited, because covering them would mean scanning every instance-partitions / task znode in the cluster on every delete; this is called out in the javadoc.
Status codes
Both guards report 409 CONFLICT rather than 500. A concurrent delete, or a create racing an in-flight delete, is expected and retryable, not a server fault, and a 500 would register as an outage in monitoring.
addTable()already rethrowsControllerApplicationExceptionuntouched; the DELETE handler wrapped every exception into a 500, so it now rethrows a deliberate status rather than downgrading it.PinotHelixResourceManageralready throwsControllerApplicationExceptionelsewhere, so this follows the existing pattern.Ordering
The materialized-view dependency check now runs before the marker is acquired, so a refused delete does not leave a marker that makes an unrelated
addTable()report a deletion in progress.Minor: segment move loop in
SegmentDeletionManagerThe
retention > 0path previously called a per-segment helper that did anexists()check before each move and logged one line per segment.getFileToDeleteURI()already establishes that the file exists, so the inner check is redundant; removing it saves one round trip per segment and the logging is now aggregated.PinotFShas no batch move, so this is still onemove()per segment — the saving is oneexists()call and one log line per segment, not an order-of-magnitude change. Also fixed:touch()is now only called on destinations whose move actually succeeded, since touching a failed destination can materialize an empty object on object stores and leave a phantom entry for the retention manager to track.What this does NOT fix
addTable()guard is still TOCTOU. The marker check is a point-in-time read; a delete can begin immediately after it passes. This narrows the window substantially but does not close it. Closing it properly needs table epoch / creation-UUID fencing, which is not in this PR.removeResourceConfigFromPropertyStore).DELETEstill proceeds when the table does not exist, by design inPinotTableRestletResource. Unchanged here.Testing
TableDeletionMarkerTest(new, 16 tests) against an embeddedZkStarterserver: acquisition, duplicate rejection, marker contents, expiry boundaries, malformed markers, stale takeover, ownership-checked release, per-table independence, and two controllers racing to acquire the marker via two independent property stores.SegmentDeletionManagerTest: added a regression test for the touch-on-failed-move bug, verified to fail before the fix and pass after. Existing 8 tests still pass.PinotTableRestletResourceTest(27 existing tests) passes, covering the modified DELETE handler including the 404-when-the-table-does-not-exist path.pinot-commonandpinot-controllercompile, the above suites pass,checkstylereports 0 violations,license:checkclean,spotless:applyapplied.Known gap in test coverage: the single-winner test asserts the invariant but is not a regression test for the non-atomic takeover it replaced. That window is sub-millisecond and could not be hit reliably from a test, and a ZK data watch cannot distinguish remove-then-create from an in-place set because zkclient dispatches on the node's state at event-processing time. The takeover's correctness rests on the ZooKeeper version check itself rather than on a red-then-green test, and is worth reviewing on that basis.
Backward compatibility / operational notes
/TABLE_DELETION_IN_PROGRESS, matching the existing convention (/SEGMENTS,/MINION_TASK_METADATA, …). Nothing in the repo enumerates the property store root, so no existing tooling is affected.POST /tablesandDELETE /tables/{tableName}can now fail with HTTP 409 CONFLICT and "a deletion is in progress" where they previously proceeded. This is the intended behaviour change and may warrant therelease-noteslabel.ZKMetadataProvider.removeTableDeletionMarkertakes an owner id (added in this PR, no external callers).Files changed
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.javapinot-common/src/test/java/org/apache/pinot/common/metadata/TableDeletionMarkerTest.java(new)pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.javapinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.javapinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.javapinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.javaSuggested labels:
bug,release-notes