Repository navigation
Fix flaky Java tests - #40298
Fix flaky Java tests#40298reuvenlax wants to merge 8 commits into
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
d79a0e8 to
8b2c024
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40298 +/- ##
============================================
+ Coverage 56.15% 58.55% +2.40%
- Complexity 2290 13732 +11442
============================================
Files 1121 2579 +1458
Lines 177169 270129 +92960
Branches 1488 11173 +9685
============================================
+ Hits 99489 158183 +58694
- Misses 75158 105947 +30789
- Partials 2522 5999 +3477
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @kennknowles for label java. This pull request likely touches a core component ("core" label). Please review with scrutiny. 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). |
Abacn
left a comment
Abacn
left a comment
There was a problem hiding this comment.
Thanks, there are many independent fixes, and some are safe while other may need some consideration. Two main comments
- Side effect of retry
- Some findings here may have already been fixed on HEAD recently
| sparkConf.setJars(filterFilesToStage(options, SPARK_JAR_EXCLUDES)); | ||
| // Enable `userClassPathFirst` to prevent issues with guava, jackson and others. | ||
| sparkConf.setIfMissing("spark.executor.userClassPathFirst", "true"); | ||
| if (!master.startsWith("local")) { |
There was a problem hiding this comment.
Some flakiness should be fixed by #40270 already
There was a problem hiding this comment.
Yes, I think I can revert this change.
| cancelSparkJobs.run(); | ||
| } | ||
| } finally { | ||
| if (cancelRequested.get()) { |
There was a problem hiding this comment.
This sounds like related to #40101, which has been fixed recently
There was a problem hiding this comment.
BTW pr/40103 (fix for 40101) actually introduced some of the flaky tests that this PR tries to address, so the flakes seen by this PR postdate 40103.
| executor.stop(); | ||
| if (attempt < MAX_START_ATTEMPTS | ||
| && (!alive | ||
| || (e.getMessage() != null && e.getMessage().contains("JobService/Prepare")))) { |
There was a problem hiding this comment.
Retry can reduce flakiness in general. However on genuine failure, keep retrying only delays the program execution. We should only retry transient failures (e.g. port conflict)
There was a problem hiding this comment.
Do we expect genuine failures on starting the Prism runner?
There was a problem hiding this comment.
IIUC Java's prism runner wrapper downloads a prism binary if not already exist in .cache/apache_beam. If downloads fail (no access to github.com; platform not have pre-build binary; run on a dev version and the binary not built separately; blocked by Stanta; etc) it could fail
…ructuredStreamingRunner
…efaultExpansionServiceClientFactory
…e Write API errors in ITs
…eIT, and SpannerChangeStreamPlacementTablePostgresIT
…d KafkaToPubsubE2ETest
12e4e1c to
d687e01
Compare
|
Reminder, please take a look at this pr: @kennknowles |
|
R: @Abacn Feel free to re-add me if you want. Parting comment/question: I have not been following this specific PR. Is it being broken into separate PRs or just multi-commit on this PR? |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
friendly ping! |
|
R: @Abacn |
|
Hi @reuvenlax, frinedly ping to resolve conflicts. Thanks |
derrickaw
added
Next Action: Author
and removed
Next Action: Reviewers
slow-review
labels
No description provided.