Tq more changes - #31542
Open
WillemKauf wants to merge 9 commits into
Open
Conversation
Adds a 5-way parity test between `redpanda` topics and `Kafka` for timequeries. This test produces a byte-identical workload (including non-monotonic timestamp phases - duplicates, jitter, a future spike followed by a long backfill below the running max) to four different `redpanda.storage.mode` backed topics (local, tiered_v1, tiered_v2, cloud) and to an Apache Kafka cluster. Then, timequeries are issued to each, and we assert all five topics agree. Timequeries run at three lifecycle points. First, when all data is expected to be local, then, after data moves to remote/L1 storage and finally, after a full cluster restart. Retention is infinite everywhere so answers cannot shift during the test.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves correctness and performance of timestamp-based offset lookup (“timequery” / Kafka ListOffsets-by-timestamp) across local storage and cloud/tiered paths by (1) preventing non-data batches (wall-clock timestamps) from contaminating timestamp bounds and (2) making time indexes safe for non-monotonic producer timestamps via running-max timestamp indexing. Adds unit/integration tests plus a ducktape parity test and a microbenchmark to validate behavior across storage modes.
Changes:
- Make timestamp seek/indexing robust: ignore non-data batches for timestamp bounds/monotonicity and introduce “running max timestamps” semantics for time indexes.
- Update cloud remote segment indexing + remote_partition timequery behavior to reduce unnecessary segment hydration/prefetch and enable safe seek starts.
- Add extensive tests/benchmarks: local storage unit tests, storage e2e coverage, cloud cost tests, and a 5-way ducktape parity test vs Apache Kafka.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rptest/tests/timequery_parity_test.py | New ducktape test generating a deterministic workload and verifying ListOffsets-by-timestamp parity across Kafka and Redpanda storage modes at multiple lifecycle points. |
| src/v/storage/tests/timequery_test.cc | Adds unit tests for monotonicity bookkeeping (non-data batches, unset max_timestamp) and for disordered timestamps using the index safely. |
| src/v/storage/tests/timequery_bench.cc | Adds a Seastar perf benchmark for timequery under different log shapes (non-data batches, disordered timestamps, interleaving). |
| src/v/storage/tests/storage_e2e_test.cc | Adds e2e regression test ensuring offset_range_size timestamp bounds ignore non-data batches. |
| src/v/storage/tests/index_state_test.cc | Updates random index_state generation to respect the new running_max_timestamps field for older encodings. |
| src/v/storage/tests/BUILD | Registers the new timequery benchmark target. |
| src/v/storage/segment_index.h | Exposes running-max and “time index is sorted” predicates to gate binary search usage correctly. |
| src/v/storage/segment_index.cc | Adjusts maybe_track monotonicity bookkeeping to consider only user-data batches. |
| src/v/storage/offset_to_filepos.cc | Adjusts uploaded range timestamp selection to consider only raft_data batches. |
| src/v/storage/index_state.h | Bumps serde version and introduces running_max_timestamps state field and version marker. |
| src/v/storage/index_state.cc | Implements running-max timestamp indexing semantics and adjusts non-data timestamp reset logic. |
| src/v/storage/disk_log_impl.cc | Makes offset_range_size timestamp bounds data-only and updates timequery reader gating to use “sorted time index” predicate. |
| src/v/cluster/partition.cc | Refines local vs cloud timequery fallback conditions when cloud retains offsets below local start. |
| src/v/cluster/archival/adjacent_segment_run.cc | Fixes merged run max_timestamp to bound all segments (use max). |
| src/v/cloud_storage/tests/timequery_cost_test.cc | New cost-focused test measuring request/parse cost impact of inflated (walltime) manifest timestamps vs data-only timestamps. |
| src/v/cloud_storage/tests/remote_segment_index_test.cc | Adds tests for find_timestamp semantics (running-max seeks) and behavior on non-monotonic legacy indexes; adds builder running-max coverage. |
| src/v/cloud_storage/tests/BUILD | Registers the new timequery_cost_test. |
| src/v/cloud_storage/remote_segment_index.h | Adds running-max timestamp tracking state in the remote segment index builder. |
| src/v/cloud_storage/remote_segment_index.cc | Writes running-max timestamps into the remote segment time index to keep it monotonic. |
| src/v/cloud_storage/remote_partition.cc | Skips segments proven unmatchable by manifest timestamps and disables prefetch for timestamp-seek readers. |
| src/v/cloud_storage/offset_index.h | Updates find_timestamp contract/docs to reflect “safe scan start” semantics and legacy non-monotonic fallback. |
| src/v/cloud_storage/offset_index.cc | Reworks find_timestamp to enforce/validate monotonicity and to return the last strictly-below entry as a safe scan start. |
Suppressed comments (1)
src/v/storage/disk_log_impl.cc:2486
- Same as above: normalize
b.header().max_timestampwithb.header().first_timestampbefore folding intomax_timestamp, to handle batches where max_timestamp is unset.
if (user_data) {
*max_timestamp = std::max(
*max_timestamp, b.header().max_timestamp);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+45
to
+46
| _running_max_timestamp = std::max( | ||
| _running_max_timestamp, hdr.max_timestamp); |
Comment on lines
+216
to
+218
| if (hdr.type == model::record_batch_type::raft_data) { | ||
| max_data_ts = std::max(max_data_ts, hdr.max_timestamp); | ||
| } |
Comment on lines
+2472
to
2475
| if (user_data) { | ||
| *max_timestamp = std::max( | ||
| *max_timestamp, b.header().max_timestamp); | ||
| } |
Collaborator
Retry command for Build#88600please wait until all jobs are finished before running the slash command |
Collaborator
CI test resultstest results on build#88600
test results on build#88607
test results on build#88608test results on build#88634test results on build#88648
|
Collaborator
Retry command for Build#88607please wait until all jobs are finished before running the slash command |
The `offset_index` time column stored each sampled batch's own max timestamp, and `find_timestamp()` returned the last entry below the query. Both assume monotonic batch timestamps. Write the running maximum data batch timestamp into the index instead, which makes entries monotonic by construction. Found by the 5-way timequery parity test in tests/rptest/tests/timequery_parity_test.py. The running maximum folds in `model::batch_max_timestamp`, added here, rather than `hdr.max_timestamp` directly: some clients leave that field unset on a batch holding a single record, and folding it raw leaves the maximum at `missing()`, which would defeat the monotonicity this relies on.
`partition::local_timequery()` discarded any local hit with result->time > cfg.time when cloud fallback was allowed, treating it as a potential miss on data below the local log start (issue 9669). But that condition is the normal outcome whenever the query time falls between two record timestamps, so on tiered topics nearly every timequery was re-answered by the remote path, which is wasted work when both agree, and a wrong answer when producers use non-monotonic timestamps and the remote index seek was unsafe. Only offsets below the local log start that are still retained in cloud storage can contain an earlier match, so make that the sole fallback condition. Found by the 5-way timequery parity test in `tests/rptest/tests/timequery_parity_test.py`. The discarded local hit sent the query to the remote path, where the segment index seek then answered too late. Reproducible with all data still local, since nothing had been trimmed.
WillemKauf
force-pushed
the
tq_more_changes
branch
from
August 12, 2026 01:43
33d723a to
5d58622
Compare
Collaborator
Retry command for Build#88608please wait until all jobs are finished before running the slash command |
WillemKauf
force-pushed
the
tq_more_changes
branch
from
August 12, 2026 15:03
5d58622 to
d2be4dd
Compare
Collaborator
Retry command for Build#88634please wait until all jobs are finished before running the slash command |
WillemKauf
force-pushed
the
tq_more_changes
branch
from
August 12, 2026 16:41
d2be4dd to
0b46744
Compare
The `base_timestamp`/`max_timestamp` an uploaded `segment` carries in the `partition_manifest` is derived from whichever batch happens to sit at the edge of the upload range, of any type. Internal batch types (`raft_configuration`, `archival_metadata`) are stamped with walltime, which can totally mess up this timestamp. Local storage already refuses to let config batches stamp a local segment for exactly this reason. To make matters worse, `partition_manifest::timequery()` picks the first segment at or above the query and the reader then walks forward, hydrating each segment until a `raft_data` batch matches. Every segment with timestamps derived from internal batches may cost a hydration just to reject the query, turning one timequery into a scan of the whole log. Instead, populate `max_timestamp` in these objects from `raft_data` batches only. A batch below the range must not describe where the range begins either. The left scan walks from an index entry up to the range's first offset, and it was recording every data batch it passed, so the last one before the range won - naming a time from outside it, and one that can sit above the range's own maximum. Record only the batch at or after the range's start, the single visited batch actually inside it; where that batch is not data, the segment's own base stands, which under-reports rather than borrowing a time from elsewhere. Under-reporting a base is safe: it only stops a timequery selecting the segment early, which max_timestamp governs. The compacted-reupload seek needs the same rule. `convert_begin_offset_to_file_pos` took the timestamp of whichever batch sat at the range's start, of any type, so a configuration or archival metadata batch there put walltime on the range - a time the range does not cover, and one that can sit above its own maximum. Let only a data batch describe the start; otherwise the caller's seed, the segment's own data-only base, stands and under-reports instead.
`partition_manifest::timequery()` selects the first segment at or above the query, but once the reader exhausts it the walk simply steps to the next segment by offset. Nothing consults the manifest again, so a timequery whose answer is far ahead or absent will hydrate every segment in between, one at a time, all the way to max_offset. For a `ListOffsets` query that bound is the high watermark, making the cost of a single timequery proportional to the size of the log. Skip any segment whose `max_timestamp` is below the query. Also, suppress small segment prefetching for timequeries, which is an ultimately useless/harmful thing to do. Measured with the timequery cost test over a 40 segment log where only 11 segments are manifest candidates. GET counts are exact; elapsed is context only. ┌────────┬──────────────┬────────────┬──────────────┬─────────┐ │ walk │ segment GETs │ index GETs │ bytes parsed │ elapsed │ ├────────┼──────────────┼────────────┼──────────────┼─────────┤ │ before │ 40 │ 40 │ 126778 │ 157ms │ ├────────┼──────────────┼────────────┼──────────────┼─────────┤ │ after │ 11 │ 11 │ 34930 │ 46ms │ └────────┴──────────────┴────────────┴──────────────┴─────────┘ The test now demonstrates/asserts the cost is bounded by the proper candidate count rather than by the total length of the log. Skipping must stop short of the end of the manifest. Reaching `manifest_end` returns "no such segment", which the caller reports as end of log - the cursor only advances to the next spillover manifest once a reader has been exhausted - so skipping the whole remainder of a manifest would end the read while the match sat in the next one. Where everything left is skippable, fall through to the segment that would have been read anyway and let its exhaustion carry the cursor over the boundary. The manifest iterator is not copyable, so the lookahead counts on a second one.
The cases:
* only_data Monotonic data and nothing else: what a timequery costs
when the index is plainly usable.
* non_data One walltime `raft_configuration` batch after the
first data batch, so that we can be sure we don't
regress timequeries when non-`raft_data` batches are
present.
* disordered Non-monotonic data.
* interleaved A walltime `archival_metadata` batch before every
data batch, as would be typical in a `tiered`
topic.
We stamp `_last_batch_max_timestamp = std::max(hdr.first_timestamp, hdr.max_timestamp);` only after updating the index's monotonicty check. Since we are going to compare future headers using this value anyways, we may as well assign it before the check and not pessimize this case. Benchmark: ``` //src/v/storage/tests:timequery_rpbench, non_data case: ┌───────────────┬──────────────┬─────────────┬───────┐ │ metric │ before │ after │ ratio │ ├───────────────┼──────────────┼─────────────┼───────┤ │ runtime │ 903.87 µs │ 19.01 µs │ 48× │ ├───────────────┼──────────────┼─────────────┼───────┤ │ allocations │ 2619 │ 95 │ 28× │ ├───────────────┼──────────────┼─────────────┼───────┤ │ instructions │ 1.82 M │ 55.1 k │ 33× │ └───────────────┴──────────────┴─────────────┴───────┘ ```
The time column recorded each indexed batch's own individual maximum timestamp, so it was only sorted when the batches happened to arrive in timestamp order. Timequeries can only use the index when this is the case. Otherwise, we do an (relatively) expensive segment scan. As of `index_state` `v12`, record the _running_ maximum over the segment's data batches instead. This makes the column sorted by construction however disordered the data is. Indices written before this carry per-batch maxima and retain their old logic. Benchmark: ``` /src/v/storage/tests:timequery_rpbench, disordered case: ┌───────────────┬──────────────┬─────────────┬───────┐ │ metric │ before │ after │ ratio │ ├───────────────┼──────────────┼─────────────┼───────┤ │ runtime │ 925.92 µs │ 18.92 µs │ 49× │ ├───────────────┼──────────────┼─────────────┼───────┤ │ allocations │ 3095 │ 97 │ 32× │ ├───────────────┼──────────────┼─────────────┼───────┤ │ instructions │ 1.94 M │ 54.2 k │ 36× │ └───────────────┴──────────────┴─────────────┴───────┘ ``` The running maximum also repairs the bound an uploaded range carries. `offset_range_size` derives it from two partial scans, each starting at an index entry, so a batch in the middle of the range is visited by neither and the largest timestamp can sit there unseen. A range whose max_timestamp under-reports makes `partition_manifest::timequery` pass over the segment holding the first matching record. The entry a scan starts from now carries the maximum over everything before it, which closes that gap. `convert_end_offset_to_file_pos` gets the same treatment for the same reason: it scans from an index entry near the range's end, so the largest timestamp in the range can sit before that entry, unseen. Bounding by the entry's running maximum keeps it from under-reporting. Note the bound is loose - the entry is near the range's end, so it reaches back over most of the segment - which is safe for a query but does make a segment look newer than its data to time-based retention.
WillemKauf
force-pushed
the
tq_more_changes
branch
from
August 12, 2026 17:02
0b46744 to
f1eeb58
Compare
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
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.
WIP
Backports Required
Release Notes