Repository navigation
Make ValidatesRunner faster: Tune Dataflow ValidatesRunner configuration #40323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,6 +174,9 @@ def legacyPipelineOptions = [ | |
| "--region=${gcpRegion}", | ||
| "--tempRoot=${dataflowValidatesTempRoot}", | ||
| "--dataflowWorkerJar=${dataflowLegacyWorkerJar}", | ||
| "--numWorkers=1", | ||
| "--maxNumWorkers=1", | ||
| "--diskSizeGb=30", | ||
| "--usePublicIps=false", | ||
| "--experiments=enable_lineage" | ||
| ] | ||
|
|
@@ -192,6 +195,9 @@ def runnerV2CommonPipelineOptions = [ | |
| "--tempRoot=${dataflowValidatesTempRoot}", | ||
| "--experiments=use_unified_worker,use_runner_v2", | ||
| "--firestoreDb=${firestoreDb}", | ||
| "--numWorkers=1", | ||
| "--maxNumWorkers=1", | ||
| "--diskSizeGb=30", | ||
| "--usePublicIps=false", | ||
| "--experiments=enable_lineage" | ||
| ] | ||
|
|
@@ -228,6 +234,18 @@ def commonRunnerV2ExcludeCategories = [ | |
| 'org.apache.beam.sdk.testing.UsesBoundedTrieMetrics', // Dataflow QM as of now does not support returning back BoundedTrie in metric result. | ||
| ] | ||
|
|
||
| def isValidatesRunnerTestClass = { FileTreeElement element -> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (element.isDirectory()) { | ||
| return true | ||
| } | ||
| if (!element.name.endsWith('.class') || | ||
| element.name.startsWith('ValidateRunnerXlangTest')) { | ||
| return false | ||
| } | ||
| return new String(element.file.bytes, java.nio.charset.StandardCharsets.ISO_8859_1) | ||
| .contains('Lorg/apache/beam/sdk/testing/ValidatesRunner;') | ||
| } | ||
|
|
||
| def createLegacyWorkerValidatesRunnerTest = { Map args -> | ||
| def name = args.name | ||
| def pipelineOptions = args.pipelineOptions ?: legacyPipelineOptions | ||
|
|
@@ -245,6 +263,7 @@ def createLegacyWorkerValidatesRunnerTest = { Map args -> | |
| classpath = configurations.validatesRunner | ||
| testClassesDirs = files(project(":sdks:java:core").sourceSets.test.output.classesDirs) + | ||
| files(project(project.path).sourceSets.test.output.classesDirs) | ||
| include isValidatesRunnerTestClass | ||
| useJUnit { | ||
| includeCategories 'org.apache.beam.sdk.testing.ValidatesRunner' | ||
| commonLegacyExcludeCategories.each { | ||
|
|
@@ -278,6 +297,7 @@ def createRunnerV2ValidatesRunnerTest = { Map args -> | |
| classpath = configurations.validatesRunner | ||
| testClassesDirs = files(project(":sdks:java:core").sourceSets.test.output.classesDirs) + | ||
| files(project(project.path).sourceSets.test.output.classesDirs) | ||
| include isValidatesRunnerTestClass | ||
| useJUnit { | ||
| includeCategories 'org.apache.beam.sdk.testing.ValidatesRunner' | ||
| commonRunnerV2ExcludeCategories.each { | ||
|
|
@@ -466,8 +486,13 @@ task validatesRunner { | |
| 'org.apache.beam.sdk.transforms.ParDoLifecycleTest.testTeardownCalledAfterExceptionInStartBundle', | ||
| 'org.apache.beam.sdk.transforms.ParDoLifecycleTest.testTeardownCalledAfterExceptionInStartBundleStateful', | ||
| ], | ||
| // Batch legacy worker does not support bundle finalization. | ||
| excludedCategories: [ 'org.apache.beam.sdk.testing.UsesBundleFinalizer', ], | ||
| // Batch legacy worker does not support bundle finalization, triggered side inputs, or unbounded PCollections. | ||
| excludedCategories: [ | ||
| 'org.apache.beam.sdk.testing.UsesBundleFinalizer', | ||
| 'org.apache.beam.sdk.testing.UsesTriggeredSideInputs', | ||
| 'org.apache.beam.sdk.testing.UsesUnboundedPCollections', | ||
| 'org.apache.beam.sdk.testing.UsesUnboundedSplittableParDo', | ||
| ], | ||
| )) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3356,7 +3356,8 @@ public void testOrderedListStateBounded() { | |
| ValidatesRunner.class, | ||
| UsesStatefulParDo.class, | ||
| UsesOrderedListState.class, | ||
| UsesOnWindowExpiration.class | ||
| UsesOnWindowExpiration.class, | ||
| UsesUnboundedPCollections.class | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }) | ||
| public void testOrderedListStateUnbounded() { | ||
| testOrderedListStateImpl(true); | ||
|
|
@@ -3420,7 +3421,12 @@ public void testOrderedListStateRangeFetchBounded() { | |
| } | ||
|
|
||
| @Test | ||
| @Category({ValidatesRunner.class, UsesStatefulParDo.class, UsesOrderedListState.class}) | ||
| @Category({ | ||
| ValidatesRunner.class, | ||
| UsesStatefulParDo.class, | ||
| UsesOrderedListState.class, | ||
| UsesUnboundedPCollections.class | ||
| }) | ||
| public void testOrderedListStateRangeFetchUnbounded() { | ||
| testOrderedListStateRangeFetchImpl(true); | ||
| } | ||
|
|
@@ -3493,7 +3499,12 @@ public void testOrderedListStateRangeDeleteBounded() { | |
| } | ||
|
|
||
| @Test | ||
| @Category({ValidatesRunner.class, UsesStatefulParDo.class, UsesOrderedListState.class}) | ||
| @Category({ | ||
| ValidatesRunner.class, | ||
| UsesStatefulParDo.class, | ||
| UsesOrderedListState.class, | ||
| UsesUnboundedPCollections.class | ||
| }) | ||
| public void testOrderedListStateRangeDeleteUnbounded() { | ||
| testOrderedListStateRangeDeleteImpl(true); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine. Validate runner tests are almost have very few data and Dataflow spins up single worker.