-
Notifications
You must be signed in to change notification settings - Fork 82
dekaf: Fix partially suspended journals returning zero offsets #2358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dekaf: Fix partially suspended journals returning zero offsets #2358
Conversation
Partially suspended journals were incorrectly returning offset 0 for both earliest and latest positions, preventing Kafka consumers from reading available data. The issue occurred because fragment listings with `begin_mod_time=i64::MAX` return empty for suspended journals. For partially suspended journals, we now: * Return `suspend.offset` as the latest position (the write head) * Continue using fragment listings for the earliest position (readable data still exists)
crates/dekaf/src/topology.rs
Outdated
| } | ||
| } | ||
| _ => { | ||
| // If fully suspended, return the suspend offset. There will be no fragments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is right... We don't have any notion of a timestamp in the partition.spec.suspend data, so what if the requested timestamp is way in the past? Maybe this really should return 0? Or an error message like OFFSET_NOT_AVAILABLE 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose you have a kafka partition with no current records in it, but which has had data in the past (so offset is != zero). How does it behave? We should do the same.
I'd guess that right behavior is returning partition.spec.suspend.offset, but that's ☝️ the real question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the full Kafka codebase, Gemini says says that the Kafka server returns UNKNOWN_OFFSET (-1) when asked to list offsets at a positive timestamp for a topic that has had data written to it, but that data has since expired out due to the retention policy:
Here's the function that builds that response:
def buildErrorResponse(e: Errors, partition: ListOffsetsPartition): ListOffsetsPartitionResponse = {
new ListOffsetsPartitionResponse()
.setPartitionIndex(partition.partitionIndex)
.setErrorCode(e.code)
.setTimestamp(ListOffsetsResponse.UNKNOWN_TIMESTAMP)
.setOffset(ListOffsetsResponse.UNKNOWN_OFFSET)
}And here's where it's called when no offsets can be found:
} else if (resultHolder.timestampAndOffsetOpt.isEmpty && resultHolder.futureHolderOpt.isEmpty) {
// This is an empty offset response scenario
resultHolder.maybeOffsetsError.map(e => throw e)
ListOffsetsPartitionStatus.builder().responseOpt(Optional.of(buildErrorResponse(Errors.NONE, partition))).build()
}I also asked about -1 and -2, and the behavior we have here matches: return the known high-watermark, aka partition.spec.suspend.offset.
Positive Timestamp
When a consumer provides a specific positive timestamp, the broker searches its log segments to find the first message with a timestamp greater than or equal to the one provided.
In this scenario, since all log segments have been deleted by retention, the partition contains no data. Therefore, the broker's search will find no matching message for any positive timestamp.
The code in KafkaApis#handleListOffsetRequest and ReplicaManager#fetchOffset shows that when no offset is found for a given timestamp, the broker constructs a response for that partition with an offset of -1 (UNKNOWN_OFFSET) and a timestamp of -1 (UNKNOWN_TIMESTAMP).
Behavior: The API call will return a response indicating that no offset could be found for the given timestamp. The response for the partition will have an offset of -1. This signals to the consumer that there are no messages at or after the requested timestamp.
crates/dekaf/src/topology.rs
Outdated
| // No fragments found. This can happen if there are no fragments at all, or if the requested timestamp is | ||
| // after the latest fragment's begin_mod_time and there is no currently open fragment. In this case, return | ||
| // the high-water mark as the requested timestamp is beyond any known offset. | ||
| None => self.fetch_write_head(partition_index).await?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the right behavior:
- The request timestamp is > any existing fragment's
begin_mod_time, and there just isn't an open fragment since the journal hasn't seen writes for longer than its flush interval, or is partially suspended or whatever the reason may be. In that case, the high-water-mark seems to me to be the clearly correct response - There are no fragments at all, but the journal isn't fully suspended. This must be because all of the fragments have expired or were deleted from cloud storage. What will a
ReadRequest's metadata response return here? Whatever it is, I feel like that's the right thing to return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a suspended journal cannot have a write head different from spec.suspend.offset. If you issue a ReadRequest, it will give you that same offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I was wrong here. Specifically, the cases where this line hits are:
suspend::Level::Partialso there is no open fragment, and the provided timestamp is after any existing persisted fragment'smod_time(and there cannot be an open fragment since the journal is partially suspended)- Not suspended, and either all fragments have expired from cloud storage, no data has ever been written, or the provided timestamp is after any persisted fragment's
mod_timeand there is no open fragment (maybe the collection hasn't seen any new data for longer than its flush interval?)
I now believe that both of these cases are the same case as above when the journal is fully suspended: a request for offsets when there are no covering fragments. As I discovered above, Kafka returns UNKNOWN_OFFSET (-1) in this case, so I believe that Dekaf should too.
The Kafka server returns `UNKNOWN_OFFSET` (-1) when asked to list offsets at a positive timestamp for a topic that has had data written to it, but that data has since expired out of retention
48ab4b1 to
5fdf23a
Compare
jgraettinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description:
Partially suspended journals were incorrectly returning a value of 0 as the high-watermark, confusing some consumers and preventing them from reading data. The issue occurred because fragment listings with
begin_mod_time=i64::MAXreturn empty for partially suspended journals, since there is no open fragment.The new behavior is now:
suspend::Level::Full:suspend.offsetsuspend::Level::Partial | suspend::Level::None: fragment listing withbegin_mod_time = 0, return 0th fragment'sbeginsuspend::Level::Full | suspend::Level::Partial:suspend.offsetsuspend::Level::None: write offset returned by non-blocking read at offset = -1begin_mod_time. The behavior ought to be the same, but this is more correct.Testing
Collection:
joseph/test-dekaf-migrations/eventsMaterialization:
joseph/test-dekaf-migrations/dekaf-genericData Plane:
gcp: us-central1 c1Request latest offset against
dekaf-devpre-deploy, pre-suspendSuspending
Request latest offset against
dekaf-devpre-deploy, post-suspend: ❌Request latest offset against
dekaf-devpost-deploy, post-suspend: ✅This change is