Kafka Streams runner: add a flush marker payload variant - #40068
Kafka Streams runner: add a flush marker payload variant#40068junaiddshaukat wants to merge 2 commits into
Conversation
First step toward bundles bounded by time (apache#39633). A bundle cannot be closed from a punctuator, because transactions are committed by the Kafka Streams runtime in the background and are not exposed, so instead a source will emit a marker that travels the topology as an ordinary record and a stage closes its bundle from process(). This adds the marker itself and nothing that emits or consumes one yet. It carries the producing partition and that transform's partition count, which is what will let it be addressed to a slice of the downstream partitions rather than broadcast: broadcasting would deliver one flush per upstream partition, so a downstream partition would see N times more flushes than the configured interval.
|
Assigning reviewers: R: @kennknowles added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
R: @je-ik |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
je-ik
left a comment
je-ik
left a comment
There was a problem hiding this comment.
I have one design question - should we just compute target partition and maybe leave the payload empty, because it only signals to the downstream transform it should close bundle?
| // would deliver one flush per upstream partition, so a downstream partition would see N times | ||
| // more flushes than the configured interval. Instead the producing partition addresses a slice | ||
| // of the downstream partitions, and the slices tile the whole range, so each downstream | ||
| // partition receives exactly one flush per interval. |
There was a problem hiding this comment.
I'd suggest making the comments more concise.
There was a problem hiding this comment.
Made it concise, thanks for pushing on it
| * partitions and the slices tile the range, so each downstream partition gets exactly one flush per | ||
| * interval. Unlike a watermark, a flush needs no aggregation on arrival: there is nothing to hold | ||
| * and nothing to combine, because only one arrives. | ||
| */ |
| int getSourcePartition(); | ||
|
|
||
| /** How many partitions the producing transform has in total. */ | ||
| int getTotalSourcePartitions(); |
There was a problem hiding this comment.
If we are to compute the target partition(s!), we need source partition, number of source partitions and number of target partitions. Given that in some cases this can produce target partition as null, should we instead just store the target partitions? Also - because the downstream transform does not care about this data, maybe the payload can be actually empty?
The marker now carries the partitions it is addressed to rather than the producing partition and count. The producer knows all three numbers, so it can compute the targets itself, and the consumer never needed the source information. That also removes the empty case from the wire: a producer with nothing to address emits no marker at all.
You are right that the consumer needs none of this, and carrying the targets is better. Changed. The marker now carries the target partitions. The producer computes them, which it can do because ShuffleByKeyProcessor already knows its own partition and the upstream count, and the downstream count is known at translation time. That also removes the empty case you spotted: instead of encoding an empty target set, a producer with nothing to address emits no marker at all, so the empty set is now rejected by the factory as a bug rather than represented on the wire. On making it fully empty, I checked and I do not think we can. StreamPartitioner.partitions(topic, key, value, numPartitions) gets the downstream count but no task context, so a partitioner has no way to learn which partition is calling it. Something has to travel in the payload for the routing to be possible at all. Carrying the targets is the smaller of the two, since it is what actually gets used and the rule then lives in one place instead of being re-derived on the other side. |
3 participants
Summary
First of four PRs toward bundles bounded by time (#39633). This one adds the flush marker to the payload envelope and nothing else: nothing emits a marker and nothing consumes one yet, so there is no behaviour change.
Why a marker rather than a timer
--maxBundleTimeMsis accepted today and has no effect. A bundle must be closed before its output is flushed, so it needs a time bound as well as a size one, otherwise on a sparse stream the elements already fed to it wait for the next watermark.The natural implementation, closing the bundle from a wall-clock punctuator, produces duplicate output against a real broker: a test with two chained GroupByKeys across four partitions emits its single group six times, reproducibly, and the count keeps climbing after input stops.
Asking about this on the Kafka dev list settled why. Matthias J. Sax's answer was that punctuations do not fit the exactly-once pattern of "read records, produce output, atomically commit the output plus the input offsets", that there is no supported way to run work just before a commit, and that transactions are an internal Kafka Streams concept deliberately not exposed at the API level. So the design was wrong rather than the implementation: bundles have to work without being coupled to transaction boundaries the runner does not control and cannot see.
He also corrected a mistaken note in #39633 — there is no
commitOffsetNeededflag, it iscommitNeeded, and it is set after a punctuation runs, so punctuator output is not outside the commit accounting as we had written. The issue has been corrected.The way forward is to make the flush data-driven, the way watermarks already are. A source emits a marker on the punctuator it already runs, the marker travels the topology as an ordinary record, and each stage closes its bundle when it receives one, inside
process(). Bundle boundaries then never touch transaction boundaries.What is here
A third variant alongside data and watermark:
FlushPayload, the narrowed view, mirroring the existingWatermarkPayload.KStreamsPayload.flush(sourcePartition, totalSourcePartitions), with the same range validation the watermark factory has.The marker carries the producing partition and that transform's partition count. Those exist so it can be targeted rather than broadcast. Broadcasting would deliver one flush per upstream partition, so a downstream partition would see N times more flushes than the configured interval asks for. Instead each producing partition will address a slice of the downstream partitions, and the slices tile the whole range, so every downstream partition receives exactly one flush per interval. The rule is
[floor(i*D/U), floor((i+1)*D/U))for upstream partitioni, withUupstream partitions andDdownstream, and it holds for fan-out, fan-in, equal counts and a single partition on either side. That logic lands in the next PR, in the partitioner.Unlike a watermark, a flush needs no aggregation when it arrives. There is nothing to hold and nothing to combine, because exactly one arrives.
The remaining three
GroupByKeyBroadcastPartitionerto something that covers targeting a possibly empty subset, and add the targeting rule.maxBundleTimeMs.ExecutableStageProcessorcloses and flushes its bundle when a marker arrives, then forwards it on.Testing
Both pass. Four new tests cover the serde round trip, the first and last partition as the boundaries of the range check, rejection of a partition outside its range, and that a flush cannot be read as a watermark or as data. Changing one field in the serde fails two of them, so they are testing something.