Skip to content

Fix DataflowOutputCounter calculation for ValueInEmptyWindows - #39487

Merged
shunping merged 11 commits into
apache:masterfrom
shunping:fix-gbk-metrics
Jul 30, 2026
Merged

Fix DataflowOutputCounter calculation for ValueInEmptyWindows#39487
shunping merged 11 commits into
apache:masterfrom
shunping:fix-gbk-metrics

Conversation

@shunping

Copy link
Copy Markdown
Collaborator

When processing shuffle or streaming data in Dataflow Legacy Runner (e.g., from GroupingShuffleReader or WindowingWindmillReader), KeyedWorkItems are wrapped inside a ValueInEmptyWindows (windows.size() == 0).

Previously, DataflowOutputCounter.update() counted these as 1 element. This caused inaccurate element counts because:

  1. A KeyedWorkItem can contain multiple elements.
  2. Elements may belong to multiple windows and need to be fanned out accordingly.
  3. KeyedWorkItems containing only timers were incorrectly incrementing element counters.

Internal bug id: 509652238

When processing shuffle or streaming data in Dataflow Legacy Runner
(e.g., from GroupingShuffleReader or WindowingWindmillReader),
KeyedWorkItems are wrapped inside a ValueInEmptyWindows (windows.size() == 0).

Previously, DataflowOutputCounter.update() counted these as 1 element.
This caused inaccurate element counts because:
1. A KeyedWorkItem can contain multiple elements.
2. Elements may belong to multiple windows and need to be fanned out accordingly.
3. KeyedWorkItems containing only timers were incorrectly incrementing element counters.
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@shunping
shunping marked this pull request as ready for review July 25, 2026 03:57
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@shunping
shunping requested a review from scwhittle July 25, 2026 03:57
@github-actions

Copy link
Copy Markdown
Contributor

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).

@kennknowles

Copy link
Copy Markdown
Member

Based on the description of the problem, would it make sense to find a way to factor the code so we can have separate streaming/batch implementation of the output counter? The ways they pass elements is just pretty fundamentally different.

@github-actions github-actions Bot added core and removed core labels Jul 29, 2026
@github-actions github-actions Bot added core and removed core labels Jul 29, 2026
@shunping

Copy link
Copy Markdown
Collaborator Author

Based on the description of the problem, would it make sense to find a way to factor the code so we can have separate streaming/batch implementation of the output counter? The ways they pass elements is just pretty fundamentally different.

The difference between the streaming and batch output counters really only matters during the GBK step, specifically regarding what gets wrapped inside ValueInEmptyWindows. Per your comment, I updated the PR to separate the implementations. Particularly I subclassed DataflowOutputCounter and extracted the specific routine that handles the window_size == 0 case.

PTAL! Thanks!

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.90323% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.81%. Comparing base (dca1253) to head (c1fb2d6).
⚠️ Report is 100 commits behind head on master.

Files with missing lines Patch % Lines
...runners/dataflow/worker/WindmillKeyedWorkItem.java 41.66% 13 Missing and 1 partial ⚠️
...ataflow/worker/StreamingDataflowOutputCounter.java 68.75% 3 Missing and 2 partials ⚠️
...rs/dataflow/worker/BatchDataflowOutputCounter.java 66.66% 2 Missing ⚠️
...runners/dataflow/worker/DataflowOutputCounter.java 71.42% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #39487      +/-   ##
============================================
- Coverage     58.05%   55.81%   -2.24%     
+ Complexity    13057     2272   -10785     
============================================
  Files          2518     1115    -1403     
  Lines        263812   174219   -89593     
  Branches      10765     1460    -9305     
============================================
- Hits         153164    97246   -55918     
+ Misses       104899    74518   -30381     
+ Partials       5749     2455    -3294     
Flag Coverage Δ
java 75.54% <62.90%> (+11.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added core and removed core labels Jul 29, 2026
Comment thread runners/core-java/src/main/java/org/apache/beam/runners/core/KeyedWorkItem.java Outdated
Comment thread runners/core-java/src/main/java/org/apache/beam/runners/core/ReduceFnRunner.java Outdated
@github-actions github-actions Bot added the core label Jul 29, 2026
@github-actions github-actions Bot removed the core label Jul 29, 2026
@shunping
shunping requested a review from scwhittle July 29, 2026 19:47
@github-actions github-actions Bot added core and removed core labels Jul 29, 2026
@github-actions github-actions Bot added core and removed core labels Jul 29, 2026
@github-actions github-actions Bot added core and removed core labels Jul 30, 2026

@scwhittle scwhittle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with small comment
Thanks!

@shunping
shunping merged commit 03db1a0 into apache:master Jul 30, 2026
23 checks passed
@shunping
shunping deleted the fix-gbk-metrics branch July 30, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants