You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "junaiddshaukat (via GitHub)" <gi...@apache.org> on 2026/08/01 15:30:38 UTC

[PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

junaiddshaukat opened a new pull request, #39578:
URL: https://github.com/apache/beam/pull/39578

   ## Summary
   
   Part of #18479.
   
   A bundle stayed open until the next watermark arrived. On a stream that produces steadily that means it grows without limit, and on one where watermarks are sparse the elements already fed to it are not emitted for as long as the gap lasts. `maxBundleSize` was declared as a pipeline option but nothing read it.
   
   The stage now counts the elements fed to the open bundle and closes it once the bound is reached.
   
   ## Commits at bundle boundaries
   
   Closing a bundle asks Kafka Streams to commit, so the elements a bundle consumed and the records it produced are committed together: a restart replays either the whole bundle or none of it.
   
   Worth being precise about what that does and does not give. It aligns commits *to* bundle boundaries; it does not prevent Kafka Streams from committing on its own interval part-way through a bundle. Ruling that out would mean closing the bundle from a pre-commit hook, so that the bundle is always finished before offsets are committed. The class documents this.
   
   ## `maxBundleTimeMs` is still not applied
   
   The natural implementation — close the bundle from a wall-clock punctuator once it has been open longer than the bound — does not work against a real broker, so it is not in this PR and the option documents that it currently has no effect.
   
   With the punctuator in, the integration test that runs two chained GroupByKeys across four partitions emits its single group about six times, and the count keeps climbing after the input has stopped. Without it, the count is exactly one.
   
   What that leaves out:
   
   - **Not the metrics folding.** The runner folds each bundle's metrics into the job's step map by adding them, so more bundles could in principle inflate a counter. `MetricsAcrossBundlesTest` splits the same input across many bundles and the counter is unchanged, so per-bundle reports are per-bundle.
   - **Not `ProcessorContext.commit()`.** With the punctuator disabled but the commit still requested on every close, the test passes. With the punctuator enabled but the commit request removed from it, the duplication still happens.
   
   So it appears to be closing a Fn-API bundle from a punctuator rather than from record processing, and I would rather leave a documented gap than ship behaviour whose failure mode I cannot explain. A pre-commit hook — closing the bundle from a state store's `flush()` — looks like the shape that would fix both this and the mid-bundle commit noted above, and I would like to agree the approach before implementing it.
   
   ## Testing
   
   - `BundleBoundaryTest` — a bound the input passes several times over produces several bundles; a bound the input never reaches leaves a single one. The elements have to arrive as separate records for the bound to count them, so the pipelines read from a `Create` rather than fanning out inside one stage, which fusion would collapse into a single input.
   - `MetricsAcrossBundlesTest` — a user counter is unchanged when the same input is split across many bundles.
   
   ```
   ./gradlew :runners:kafka-streams:validatesRunner       # 49 tests
   ./gradlew :runners:kafka-streams:build                 # 80 unit tests, spotless + checker + errorprone
   ./gradlew :runners:kafka-streams:brokerIntegrationTest # 3 tests, needs Docker
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "junaiddshaukat (via GitHub)" <gi...@apache.org>.
junaiddshaukat commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3702097246


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   Yes, the time-based close will be its own PR. I pulled it out of this one because it duplicated output on a real broker and I didn't want to ship a mechanism whose failure mode I couldn't explain. Still hoping for your read on whether a state store's flush() as a pre-commit hook is the right shape for it.
   
   And you're right about the key, it should be null. Today Impulse and Read emit new byte[0] as a placeholder and every stage just carries that along, which isn't the same thing at all: an empty array is a real key, so murmur2 hashes it to one fixed partition, whereas a null key is the one Kafka spreads. So a keyless record reaching a sink would pile onto a single partition instead of being distributed — which is exactly the wrong behaviour for the rescaling numbers we want.
   
   It's masked at the moment because the only sink we have is the GroupByKey repartition topic, and data reaching it goes through ShuffleByKeyProcessor, which overwrites the key with the encoded Beam key. So the placeholder never actually reaches a partitioner as a data key today. It would start to matter as soon as a stage output goes to a topic directly.
   
   One thing I'd want to fix as part of that change: in GroupByKeyBroadcastPartitioner, partition() null-checks the key but partitions() — the one Kafka Streams actually calls — does murmur2(key) unguarded, so a null key would NPE there today.
   
   I'd rather do that as a separate PR than widen this one, since it touches Impulse, Read and the partitioner rather than bundling. Shall I file an issue and pick it up after the time bound, or would you prefer it folded in here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "je-ik (via GitHub)" <gi...@apache.org>.
je-ik commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3702033466


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   Does it mean that closing the bundle by time will come in a different PR? And out of curiosity - the stateless (and therefore keyless) ExecutableStage _should_ have null key in KafkaRecord and this null key should be passed downstream, no?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #39578:
URL: https://github.com/apache/beam/pull/39578#issuecomment-5152090096

   > [!CAUTION]
   > The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "je-ik (via GitHub)" <gi...@apache.org>.
je-ik commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3702141313


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   We can make a different issue and PR for the followups.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "je-ik (via GitHub)" <gi...@apache.org>.
je-ik commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3700049364


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   This feels weird. If the key is null, we should not inject the "last" key, because it might be wrong. Does this mean we have "unkeyed" (i.e. stateless) ExecutableStage?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39578:
URL: https://github.com/apache/beam/pull/39578#issuecomment-5152169427

   Assigning reviewers:
   
   R: @jrmccluskey added as fallback since no labels match configuration
   
   
   Note: If you would like to opt out of this review, comment `assign to next reviewer`.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "je-ik (via GitHub)" <gi...@apache.org>.
je-ik merged PR #39578:
URL: https://github.com/apache/beam/pull/39578


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "junaiddshaukat (via GitHub)" <gi...@apache.org>.
junaiddshaukat commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3700100405


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   You're right, and it's worse than unnecessary standing in a different record's key there would just be wrong.
   
   That parameter existed only because I had the bundle also being closed by a wall-clock punctuator, which has no record to take a key from, so it passed the last key it had seen. I dropped the punctuator from this PR when it turned out to duplicate output, and I left the substitution behind. It's gone now — the close takes the record again and carries that record's own key and timestamp, exactly as before this PR.
   
   And yes, to your question: an ExecutableStage is unkeyed. It runs stateless, with StateRequestHandler.unsupported() and no timer receivers, so the Kafka record key means nothing to it and is only carried along. Where the key does matter, downstream sets it — ShuffleByKeyProcessor derives it from the Beam key before a GroupByKey. I've added that to the javadoc so the next reader doesn't have to ask.
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] [GSoC 2026] Kafka Streams runner: bound a bundle by element count [beam]

Posted by "je-ik (via GitHub)" <gi...@apache.org>.
je-ik commented on code in PR #39578:
URL: https://github.com/apache/beam/pull/39578#discussion_r3700049364


##########
runners/kafka-streams/src/main/java/org/apache/beam/runners/kafka/streams/translation/ExecutableStageProcessor.java:
##########
@@ -253,6 +285,18 @@ private FnDataReceiver<WindowedValue<?>> mainInputReceiver() {
   }
 
   private void closeBundleAndFlush(Record<byte[], KStreamsPayload<?>> record) {
+    byte[] key = record.key();
+    closeBundleAndFlush(key == null ? lastKey : key, record.timestamp());

Review Comment:
   This feels weird. If the key is null, we should not inject the "last" key, because it might be wrong. Does this mean we have "unkeyed" (e.g. stateless) ExecutableStage?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org