Repository navigation
Make ValidatesRunner faster: Tune Dataflow ValidatesRunner configuration - #40323
Conversation
…tegory exclusions
| "--region=${gcpRegion}", | ||
| "--tempRoot=${dataflowValidatesTempRoot}", | ||
| "--dataflowWorkerJar=${dataflowLegacyWorkerJar}", | ||
| "--numWorkers=1", |
There was a problem hiding this comment.
We should decide whether this is ok
We can trigger https://github.com/apache/beam/actions/workflows/beam_PostCommit_Java_ValidatesRunner_Dataflow.yml
manually by modifying '.github/trigger_files/beam_PostCommit_Java_ValidatesRunner_Dataflow.json' in the PR and push a commit
See https://github.com/apache/beam/blob/master/.github/workflows/README.md#running-workflows-manually
There was a problem hiding this comment.
It will work (I've triggered it on another PR). My question is whether it changes our test coverage since we won't have multi-worker parallelism.
There was a problem hiding this comment.
I think it's fine. Validate runner tests are almost have very few data and Dataflow spins up single worker.
| UsesOrderedListState.class, | ||
| UsesOnWindowExpiration.class | ||
| UsesOnWindowExpiration.class, | ||
| UsesUnboundedPCollections.class |
There was a problem hiding this comment.
I wonder if we could have an undocumented service-side flag, or something SDK side, to reject pipelines that aren't in the expected batch/streaming mode. I know we were accidentally automatically upgrading and running streaming jobs in the batch suite before and this fixes it.
|
Assigning reviewers: R: @ahmedabu98 for label java. 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). |
|
R: @kennknowles |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| 'org.apache.beam.sdk.testing.UsesBoundedTrieMetrics', // Dataflow QM as of now does not support returning back BoundedTrie in metric result. | ||
| ] | ||
|
|
||
| def isValidatesRunnerTestClass = { FileTreeElement element -> |
There was a problem hiding this comment.
Realizing that this particular helper function is a bit wonky. Like this is definitely a case where the LLM DGAF how it gets the job done, and it'll have comprehensibility issues down the line.
Set numWorkers=1 and lower diskSize. NB: We should decide whether this is ok, or whether we want to make sure that all our tests have at least 2 workers!
Exclude streaming-only tests (UsesTriggeredSideInputs, UsesUnboundedPCollections, UsesUnboundedSplittableParDo) from the legacy ValidatesRunner task. These tests already run in the streaming ValidatesRunner. Add missing UsesUnboundedPCollections annotations.
Remove @RunWith(Enclosed.class). This enables increased parallelism (followup PR).