Repository navigation
Replies: 3 comments 4 replies
|
i skimmed thru this proposal and I like the idea. i will review this in detail this week. |
|
@djanand overall i like this. a few things need to be addressed:
for the default, i'd go opt-in. low throughput can be normal, and this changes rotation, allocation and retention for those topics. topic defaults are catalog constants, not server settings, and admission persists them on new topics. so please spell out separately what new topics get and what a missing key means for existing ones. (defaults) it also needs a lower bound. tests i'd want to see:
i think PR for this will be huge 🥲 |
|
On defaults. Opt-in, and the cleanest way I can see is OptionDefault::None with
So absent is the only off state, and there is nothing to migrate or special-case on read. On the floor/ lower bound Agreed. Worth saying first that the code already fails safe here: the offer builder checks the entry count before encoding and returns ManifestTooLarge rather than tripping the encoder's assert, with a comment noting a small configured segment size makes such a chain ordinary. So this is about catching misconfiguration early, not about preventing a crash, which is why I would keep it light. MIN_TOPIC_SEGMENT_MS of 1 minute, as the direct mirror of MIN_TOPIC_SEGMENT_SIZE
For the running case I would copy the cleaner's disclosure for an unenforceable On tests. All six. Adding one: the roll decision should return which trigger fired, otherwise a test can't tell a time roll from a size roll that happened to land in the same place. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Goal
Put a ceiling on how much time a segment spans, so retention works on partitions that do not fill a segment quickly. This is finer retention granularity, not a bound on message lifetime. [edited]
Related to #3312. A partition that never seals cannot be compressed or offloaded either, so bounding the span can be a prerequisite for the tiering work discussed there rather than a parallel concern.
Issue
Segments seal on size alone.
Segment::is_full()compares against the per-topic segment_size, 1 GiB by default, and there is no time trigger. Retention currently looks at sealed segments.Segment::is_expired()and bails early on anything unsealed. So essentially,message_expiryruns on message timestamps, continuously regardless of segment state.The expiry condition can be true for weeks without sealing the segment.
What it looks like in practice
With
segment_size = 1 GiBandmessage_expiry = 30 days:How late deletion runs
Expiration compates
max_timestamp, the newest message in a segment, so nothing is deleted until the newest message is aged out. The oldest message in a segment lives as long as the segment spans. For e.g takingmessage_expiry = 1hCurrent state
Proposed State (with segment_ms)
Setting segment_ms = 1h caps the span, and the worst case stops depending on throughput:
Worst case goes from message_expiry + however long a segment takes to fill to message_expiry + segment_ms, which is the guarantee Kafka gives with segment.ms.
High-level shape
segment_msalongsidesegment_size, parsed the same way. Rotation becomes size-or-span.Both values are primary-assigned and replicated, so every replica rolls at the same message and boundaries stay aligned. Additionally, the roll has to happen before the arriving batch is appended. If it lands in the old segment first, that segment's
max_timestampjumps forward and its contents stop being expirable for another fullmessage_expiryPreallocated segments need trimming at seal
preallocate_segmentsreserves exactlysegment_sizeup front. That is safe today because a segment only seals once it has reached or exceeded that size, so the reservation is always fully used and there is nothing to reclaim.max_topic_sizecompares againstsegment.size, which counts written bytes. It would see a few MB while the disk fillsonBecomeInactiveSegment, which callslog.trim()What this does not cover
Operation, androuter.rssays the version fence that would make that safe during a rolling upgrade is not there yet. Both are bigger than this change, so I would keep them separate.Open Questions
segment_sizeis fixed at creation becausemax_topic_sizeadmission uses it to work out the smallest cap it can enforce, which only holds if existing segments really are that size. No calculation uses a time setting that way. Mutable matters here anyway: the topics with this problem already exist, and create-only would mean recreating them to fix it.noneIORING_OP_FTRUNCATE, which only exists from kernel 6.9, and on anything older the driver reaches for a blocking pool the shard does not have and panics. Proposal is copyFileStorage::truncaterather than invent something, but I would rather hear it is the house pattern than assume. It would make a failed trim non-fatal, a warning and carry on, since the segment is perfectly good either way and you have only failed to give some blocks back.Affected area / component
Alternatives considered
segment_sizeat creation. Only helps new topics, since the key is create-only, and expresses the wrong intent by trading segment count against a size budget when the requirement is about time.What I'd like from this discussion
FileStorage::truncateis the pattern to copy for the seal-time trim, or there is a better oneContribution
[x] I'm willing to submit a pull request to implement this feature
Good first issue
[ ] unchecked - touches persistence and the rotation path
All reactions