beam icon indicating copy to clipboard operation
beam copied to clipboard

[#30083] Add processing time to prism.

Open lostluck opened this issue 1 year ago • 2 comments

Add a ProcessingTime queue to Prism's element manager, that can be appropriately controlled by the TestStream notion of time.

Basic design is that the queue doesn't contain the elements, but instead manages which stage needs to be notified when to inject queue elements into processing, and in particular, syncing it with existing watermark processing, which decides when elements are ready to be processed anyway. This allows continuing to have each stage maintain all the state for the stage, instead of splitting it between the element manager and the stages.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • [ ] Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • [ ] Update CHANGES.md with noteworthy changes.
  • [ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels Python tests Java tests Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

lostluck avatar Mar 04 '24 23:03 lostluck

Codecov Report

Attention: Patch coverage is 82.55814% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 38.55%. Comparing base (6406cfe) to head (275e307). Report is 197 commits behind head on master.

:exclamation: Current head 275e307 differs from pull request most recent head 313bc5b. Consider uploading reports for the commit 313bc5b to get more accurate results

Files Patch % Lines
...am/runners/prism/internal/engine/elementmanager.go 26.66% 10 Missing and 1 partial :warning:
sdks/go/pkg/beam/runners/prism/internal/stage.go 85.18% 2 Missing and 2 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #30492      +/-   ##
==========================================
+ Coverage   38.52%   38.55%   +0.02%     
==========================================
  Files         698      699       +1     
  Lines      102374   102439      +65     
==========================================
+ Hits        39442    39497      +55     
- Misses      61302    61307       +5     
- Partials     1630     1635       +5     
Flag Coverage Δ
go 54.37% <82.55%> (+0.05%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 21 '24 20:03 codecov[bot]

Codecov Report

Attention: Patch coverage is 82.55814% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 38.55%. Comparing base (b34cf54) to head (275e307). Report is 304 commits behind head on master.

:exclamation: Current head 275e307 differs from pull request most recent head 30c239d

Please upload reports for the commit 30c239d to get more accurate results.

Files Patch % Lines
...am/runners/prism/internal/engine/elementmanager.go 26.66% 10 Missing and 1 partial :warning:
sdks/go/pkg/beam/runners/prism/internal/stage.go 85.18% 2 Missing and 2 partials :warning:
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #30492       +/-   ##
=============================================
- Coverage     71.44%   38.55%   -32.90%     
=============================================
  Files           906      699      -207     
  Lines        113271   102439    -10832     
  Branches       1076        0     -1076     
=============================================
- Hits          80931    39497    -41434     
- Misses        30327    61307    +30980     
+ Partials       2013     1635      -378     
Flag Coverage Δ
go 54.37% <82.55%> (+0.04%) :arrow_up:
java ?

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 22 '24 21:04 codecov-commenter

R: @damondouglas

lostluck avatar May 28 '24 18:05 lostluck

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

github-actions[bot] avatar May 28 '24 18:05 github-actions[bot]

LGTM pending the latest check re-run passes; nothing really stood out for me accept for the couple of comments.

The only failure was the Dataflow run of the ProcessingTime_Bounded test, which I had not filtered out. Dataflow only properly handles processing time in Streaming pipelines,and that test pipeline executes as "batch".

lostluck avatar May 29 '24 03:05 lostluck