Repository navigation
fix(pd): make the TTL cleanup delete the expired key under its marker - #3382
bitflicker64 wants to merge 2 commits into
Conversation
clearTTLData took the key from the marker K@T@<time>@<key> at the delimiter after T, so it deleted "<time>@<key>", a name that never exists, and then removed the marker: an expired value that nobody read again stayed in the store for good. The cleanup now parses the key after the time, and deletes it only while it still holds the value the marker names: the stored value must carry the marker's expiry, and right before the delete the key must still hold the bytes the cleanup read. A renewal with another TTL, a plain put without one, or a keepAlive keeps its value; only the stale marker goes. The check and the delete are two store operations; one atomic delete needs the txn RPC of apache#3376. Fixes apache#3378
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3382 +/- ##
============================================
+ Coverage 55.93% 55.94% +0.01%
- Complexity 2277 2318 +41
============================================
Files 774 776 +2
Lines 68534 68888 +354
Branches 9333 9417 +84
============================================
+ Hits 38334 38542 +208
- Misses 26634 26760 +126
- Partials 3566 3586 +20
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The parse fix and the expiry guard are correct for every TTL a marker can sort under today, but the cleanup now deletes a live value whenever its marker falls inside the scan range without having expired, which the byte-wise range lets happen for large TTLs. Evidence: read the full diff at 75468db and KvService, MetadataRocksDBStore, HgKVStoreImpl.scanRange (RocksDB iterator bounds, default byte-wise comparator), TaskScheduleService and the putTTL/keepAlive RPCs; computed marker membership in [K@T@0@, K@T@@) for several TTLs; all non-skipped checks pass at this head.
The cleanup selects markers with a byte-wise range up to K@T@<now>@, so a time with more digits than now can sort below it: today a TTL of 10^13 ms is selected right after the put. With the parse fixed, the cleanup then deleted a value that a read still returns. removeExpired now leaves a marker whose time has not passed alone, with its value, using the expiry test of getValue. The interleaving tests pass a marker that is due, as the scan does.
1 participant
Purpose of the PR
KvService.clearTTLDataruns every second on the PD leader and should delete each key whose TTLmarker
K@T@<expireAt>@<key>has expired. It took the key from the marker at the delimiter afterT, so it deleted<expireAt>@<key>, a name that never exists, and then removed the marker. Aread hides an expired value and removes it when it finds one, but a value that nobody reads again
stayed in the store for good, with no marker left that would ever delete it. #3378 has the
measurement on master.
The parse fix alone would make the cleanup delete a key that a writer is renewing at that moment,
which the review of #3376 asked to prevent: the cleanup's delete should be bound to the expired
marker. This PR carries the parse fix and that guard without the
txnRPC of #3376, so the guardis a check before the delete, not one atomic raft entry. Once that RPC lands, the check and the
delete can become one guarded txn; its two compares are the two checks below.
This PR is limited to the TTL cleanup's parse and its guard; a bug that already exists on master in
code this PR did not write is tracked as an issue, not fixed here.
Main Changes
Two commits, two files, 59 production lines (56 added, 3 removed) and 131 test lines. The second
commit answers the review: a marker that is not due yet is left alone.
clearTTLDataparses the key after the marker's time, reads the key, and hands both to the newKvService.removeExpired(key, marker, stored), which deletes the key and the marker only while thekey still holds the value the marker names:
returns the value. The scan compares bytes, so a time with more digits than now can sort below
it; today a TTL of 10^13 ms is selected right after the put.
marker goes.
another TTL (
putTTL,keepAlive) or none (a plainput), so it is kept and the stale markergoes.
since, so the value is kept and the stale marker goes. The bytes include the expiry, so this
catches a renewal of the identical string.
The check and the delete are two store operations, so a renewal that lands in between, after the
cleanup compared the bytes and before its two removes, is still deleted. That is the window the
txnof #3376 closes.Verifying these changes
KvServiceTest(hg-pd-test,pd-core-testsuite), seven new tests:testTtlCleanupDeletesTheExpiredKeyis the issue's measurement, it fails on master's parseand passes here;
testTtlCleanupKeepsAValueRenewedBeforeItsRead,testTtlCleanupKeepsAValueRenewedAfterItsRead,testTtlCleanupKeepsALeaseAKeepAliveRenewedAfterItsReadandtestTtlCleanupKeepsAPlainPutOverATtlValueare the interleavings;testTtlCleanupDropsTheMarkerOfARemovedValueis the absent value;testTtlCleanupKeepsAValueThatHasNotExpiredis a 10^13 ms TTL whose marker the scan picks.master's parse fails
testTtlCleanupDeletesTheExpiredKey; without the expiry check,testTtlCleanupKeepsAPlainPutOverATtlValueandtestTtlCleanupKeepsAValueRenewedBeforeItsReadfail; without the byte compare,
testTtlCleanupKeepsAValueRenewedAfterItsReadandtestTtlCleanupKeepsALeaseAKeepAliveRenewedAfterItsReadfail; without the due check,testTtlCleanupKeepsAValueThatHasNotExpiredfails. All nine pass with every guard.editorconfig:checkclean, pd-common 83/0/0/0, pd-core 197/0/0/2 (190 on master, the 7new tests), pd-client 83/0/0/0 against a packaged PD (tests, failures, errors, skipped).
Does this PR potentially affect the following parts?
putTTL, PD locks, everyKvService.put(key, value, ttl)caller) now leave the store within a second of their expiry,as they were meant to. Before, an expired value stayed until a read found it; no read served it
as live either way.
Documentation Status
Select one option and provide the documentation location when applicable.
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR: