Repository navigation
Conversation
…h 2s watermark hold
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR resolves a flakiness issue in GroupByKeyTest.BasicTests.testAfterProcessingTimeContinuationTriggerUsingState caused by CPU contention under parallel test execution. Marking the test with @BeamParallelJunit4Runner.SerialTest and increasing waitMillis from 500ms to 2000ms is a clean, targeted fix that ensures processing-time triggers across both consecutive GroupByKey stages have sufficient wall-clock headroom before the event-time watermark advances.
🔍 General Feedback
- The root cause analysis and fix are sound; no functional issues were found.
- Consider updating the Javadoc comment right above the test (lines 239–241), which still states "Currently the sleep is sub-second", to stay consistent with the new 2-second sleep duration.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Updated the Javadoc comment above /gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR resolves a timing flake in GroupByKeyTest.BasicTests.testAfterProcessingTimeContinuationTriggerUsingState under parallel ValidatesRunner execution by annotating the test with @BeamParallelJunit4Runner.SerialTest and increasing the real-time watermark hold (waitMillis) from 500ms to 2000ms. The change is well-scoped, updates the corresponding Javadoc comment, and directly addresses CPU contention during wall-clock processing-time trigger propagation.
🔍 General Feedback
- Using
@BeamParallelJunit4Runner.SerialTeston this wall-clock-sensitive test insideBasicTests(@RunWith(BeamParallelJunit4Runner.class)) properly drains in-flight parallel tests in the class before running and avoids intra-class thread contention.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41281 +/- ##
============================================
- Coverage 59.26% 57.74% -1.52%
+ Complexity 4925 2633 -2292
============================================
Files 1462 1203 -259
Lines 186313 171888 -14425
Branches 3669 2182 -1487
============================================
- Hits 110417 99257 -11160
+ Misses 72540 69966 -2574
+ Partials 3356 2665 -691 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Note: this PR was auto-generated by tooling. Before moving it out of draft, the author has done a full review of the PR, specifically evaluating it on the following criteria:
Full details of the methodology of generation and review can be found here - https://docs.google.com/document/d/14lj7ECVov4T4EdwmIzNlWrRGE3lLDMOwX5xUMGzaLoc/edit?usp=sharing
If you have concerns with the PR or with the underlying methodology used to generate this, please comment below - they will be addressed by the author not the tooling :)
Summary
GroupByKeyTest.BasicTests.testAfterProcessingTimeContinuationTriggerUsingStatewith@BeamParallelJunit4Runner.SerialTestand increaseswaitMillisfrom500to2000.Root Cause
In
PostCommit Java ValidatesRunner Direct JavaVersions(run 43366517998),GroupByKeyTest$BasicTests > testAfterProcessingTimeContinuationTriggerUsingStatefailed with:testAfterProcessingTimeContinuationTriggerUsingStatecannot useTestStreamprocessing-time advancement (because it runs acrossValidatesRunnersuites), so it holds the event-time watermark using a real wall-clockThread.sleep(waitMillis)(waitMillis = 500) inside an@OnTimercallback (onDelay) while two sequential downstreamGroupByKeytransforms (Triggered sumandSecond Triggered sum) fireAfterProcessingTime.pastFirstElementInPane().plusDelayOf(Duration.millis(1))early panes (PAssert.that(triggeredSums).inEarlyGlobalWindowPanes().containsInAnyOrder(42)).After
GroupByKeyTest.BasicTestswas migrated to@RunWith(BeamParallelJunit4Runner.class)(#40292) and:runners:direct-java:validatesRunnerenabledmaxParallelForks = 4withbeam.test.parallelThreads = 8(#40324), up to 32DirectRunnerpipelines execute concurrently on a 4-vCPU runner. Under that CPU contention, propagating elements and firing processing-time timers across two consecutiveGroupByKeystages can take longer than the 500ms wall-clock sleep inonDelay. WhenonDelaywakes up first, the input watermark advances toTIMESTAMP_MAX_VALUE, causingSecond Triggered sumto emit its result in anON_TIMEpane rather than anEARLYpane.Annotating the test with
@BeamParallelJunit4Runner.SerialTestand increasingwaitMillisto2000prevents intra-class thread contention during the real-time sleep and provides sufficient headroom for both processing-time trigger stages to fire before the watermark is released.Verification
./gradlew :sdks:java:core:spotlessCheck :runners:direct-java:validatesRunner --tests "org.apache.beam.sdk.transforms.GroupByKeyTest\$BasicTests.testAfterProcessingTimeContinuationTriggerUsingState" :sdks:java:core:checkstyleTest.Verification
beam_PostCommit_Java_ValidatesRunner_Direct_JavaVersions.ymlworkflow run on branch:43379443238