feat(core): use wall clock for TTL to prevent accidental data loss#6531
feat(core): use wall clock for TTL to prevent accidental data loss#6531bluestreak01 merged 5 commits intomasterfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a configurable TTL wall-clock feature to QuestDB. Adds a new PropertyKey Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit read the description and review |
|
@bluestreak01: I'll review the PR changes now. ✅ Actions performedReview triggered.
|
bluestreak01
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/main/java/io/questdb/cairo/TableWriter.java (1)
147-150: Redundant static imports after switching toTableUtils.*.
Withimport static io.questdb.cairo.TableUtils.*;you can likely drop explicitopenAppend/openROstatic imports to avoid duplication (no functional impact).core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java (1)
280-287: Test update matches new TTL semantics; consider using the local timestamp parsing helper for consistency.
This setup correctly advancescurrentMicros(wall clock) so TTL reference time won’t be driven by “future” inserted timestamps anymore, matching the PR’smin(maxTimestamp, wallClock)behavior. Minor consistency nit: nearby code often usesparseFloorPartialTimestamp(...); using the same helper here would keep parsing consistent and reduce direct dependency onMicrosTimestampDriver.INSTANCE.core/src/main/java/io/questdb/PropServerConfiguration.java (1)
508-509: Good: config-backedttlUseWallClockis immutable and centrally owned.
No concerns with adding thisfinalfield. Consider grouping TTL-related fields together for discoverability (purely optional).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/io/questdb/PropServerConfiguration.java(3 hunks)core/src/main/java/io/questdb/PropertyKey.java(1 hunks)core/src/main/java/io/questdb/cairo/CairoConfiguration.java(1 hunks)core/src/main/java/io/questdb/cairo/TableWriter.java(11 hunks)core/src/main/resources/io/questdb/site/conf/server.conf(1 hunks)core/src/test/java/io/questdb/test/ServerMainTest.java(1 hunks)core/src/test/java/io/questdb/test/cairo/TtlTest.java(12 hunks)core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/io/questdb/cairo/TableWriter.java (2)
core/src/main/java/io/questdb/cairo/TableUtils.java (1)
TableUtils(82-2127)core/src/main/java/io/questdb/cairo/PartitionBy.java (1)
PartitionBy(44-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (17)
core/src/main/resources/io/questdb/site/conf/server.conf (1)
280-282: LGTM! Clear documentation and safe default.The new TTL configuration option is well-documented and follows existing conventions. The default value of
trueappropriately guards against accidental data loss from future timestamps, while still allowing users to opt into the legacy behavior if needed.core/src/main/java/io/questdb/cairo/TableWriter.java (6)
1269-1360: Wall-clock captured once and threaded through WAL commit + housekeeping: LGTM; verify “timestamp of record” intent.
CapturingwallClockMicrosonce (Line 1289) and reusing it forprocessWalCommit(...),lastWalCommitTimestampMicros, andhousekeep(...)achieves the syscall reduction goal. Just confirm you’re OK withlastWalCommitTimestampMicrosreflecting the start ofcommitWalInsertTransactions(...)rather than the end of the commit (could slightly skew the commit-latency heuristic).
2118-2191: Non-WAL commit path now uses a single wall clock for housekeeping: looks correct.
CapturingwallClockMicrosonce percommit(long o3MaxLag)and passing intohousekeep(wallClockMicros)is consistent with the WAL path and keeps TTL enforcement aligned to the same reference time.
4061-4114: TTL eviction guarded by wall clock: good safety fix; double-check clock-backwards behavior is acceptable.
UsingmaxTimestamp = min(maxTimestamp, timestampDriver.fromMicros(wallClockMicros))whenconfiguration.isTtlWallClockEnabled()is the right mitigation for far-future inserts. Only caveat: if the wall clock jumps backwards, TTL eviction can stall (probably acceptable given the feature’s semantics, but worth explicitly accepting).
5776-5787:housekeep(long wallClockMicros)signature change is clean and keeps TTL deterministic per-commit.
ThreadingwallClockMicrosintoenforceTtl(wallClockMicros)avoids extra clock reads and ensures all housekeeping steps in a commit share the same time base.
7593-7833: WAL commit-latency heuristic now uses passed wall clock: OK; validate units and negative deltas.
needFullCommitnow includeswallClockMicros - lastWalCommitTimestampMicros > configuration.getCommitLatency()(Line 7658). Please confirmgetCommitLatency()is in microseconds (matchingMicrosecondClock) and decide how you want to behave if the delta is negative (clock set backwards).
7835-7905: WAL helper overload updated consistently to pass wall clock through.
The wrapperprocessWalCommit(..., long wallClockMicros)correctly forwards the value into the coreprocessWalCommit(...)overload.core/src/test/java/io/questdb/test/ServerMainTest.java (1)
248-813: Expected “show parameters” output updated correctly for new TTL wall-clock property.
The added row forcairo.ttl.use.wall.clockmatches the new config key’s defaulttrueand keeps the set-based comparison consistent.core/src/main/java/io/questdb/cairo/CairoConfiguration.java (1)
789-801: Good, backward-compatible config surface for TTL wall-clock guarding.
Addingdefault boolean isTtlWallClockEnabled()with atruedefault is the right API shape for rolling this out without forcing allCairoConfigurationimplementors to update immediately.core/src/main/java/io/questdb/PropServerConfiguration.java (2)
1409-1410: Defaulttrue+getBoolean()wiring looks correct for safe-by-default behavior.
This should also naturally appear inallPairs(and thereforeSHOW PROPERTIES) via the existinggetString()tracking.
4322-4326: Accessor correctly exposes the toggle throughCairoConfiguration.
This is the right place to surface the property for downstream TTL logic.core/src/main/java/io/questdb/PropertyKey.java (1)
612-614: NewPropertyKeyconstant is safe to add; no code relies on enum ordinals.The verification script found no instances of
PropertyKey.ordinal()usage or ordinal-based branching logic in the codebase. Adding theCAIRO_TTL_USE_WALL_CLOCKentry poses no risk of breaking existing functionality.core/src/test/java/io/questdb/test/cairo/TtlTest.java (5)
27-27: LGTM! Import necessary for wall-clock configuration.The
PropertyKeyimport is correctly added to support the new test that toggles theCAIRO_TTL_USE_WALL_CLOCKconfiguration.
132-137: LGTM! Text block formatting improves readability.Converting concatenated assertion strings to Java text blocks is a good readability improvement with no semantic changes.
Also applies to: 206-212, 219-224, 550-556, 564-569, 577-582, 590-596, 604-609, 745-751, 759-764, 772-778, 786-791
227-273: Excellent test coverage for wall-clock TTL protection with TIMESTAMP type.The test correctly verifies that inserting a far-future timestamp (2100) doesn't cause premature eviction of recent data when wall-clock protection is enabled. The timestamp calculations and partition ceiling logic are accurate, and the test properly exercises both WAL and non-WAL modes.
275-321: LGTM! Proper coverage for TIMESTAMP_NS type.This test correctly validates wall-clock TTL protection for nanosecond-precision timestamps. The intentional duplication ensures both
TIMESTAMPandTIMESTAMP_NScolumn types are tested.
323-363: LGTM! Correctly tests the opt-out behavior.The test properly verifies that when wall-clock protection is disabled via
CAIRO_TTL_USE_WALL_CLOCK=false, TTL enforcement reverts to using onlymaxTimestamp, allowing far-future timestamps to cause eviction of older data. The property is correctly restored in the finally block.
bluestreak01
added
Bug
[PR Coverage check]😍 pass : 45 / 51 (88.24%) file detail
|
|
fixes #5461 |
4 participants
Summary
Adds wall-clock protection to TTL enforcement to prevent accidental data loss when future timestamps are inserted.
Problem
When a table has TTL enabled and a row with a far-future timestamp is inserted (e.g., year 2100), TTL enforcement would use that future timestamp as the reference point. This causes all existing data to appear "expired" relative to the future timestamp, leading to unexpected partition eviction.
Solution
TTL enforcement now uses
min(maxTimestamp, wallClockTime)instead of justmaxTimestamp. This ensures that inserting future timestamps cannot cause existing data to be evicted prematurely.A new configuration option
cairo.ttl.use.wall.clock(default:true) allows opting out of this behavior if needed.Additional Optimization
Reduced microsecond clock syscalls in commit paths:
processWalCommit(),housekeep(), andenforceTtl()Configuration
cairo.ttl.use.wall.clocktrueTest Plan
testFutureTimestampDoesNotWipeTable- verifies wall-clock protection workstestFutureTimestampDoesNotWipeTableNanos- same for TIMESTAMP_NStestFutureTimestampWipesTableWhenWallClockDisabled- verifies opt-out behavior