Skip to content

test(workflow-core): add @NonParallelTest to de-flake shared-MinIO suites - #7051

Merged
Yicong-Huang merged 1 commit into
apache:mainfrom
Yicong-Huang:fix/7049-serialize-minio-test-suites
Jul 29, 2026
Merged

test(workflow-core): add @NonParallelTest to de-flake shared-MinIO suites#7051
Yicong-Huang merged 1 commit into
apache:mainfrom
Yicong-Huang:fix/7049-serialize-minio-test-suites

Conversation

@Yicong-Huang

@Yicong-Huang Yicong-Huang commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

De-flake S3StorageClientSpec, which intermittently fails 4 tests in the build / amber CI job (scope WorkflowCore / Test / test).

Root cause. Four suites mix S3StorageTestBase and share a JVM-wide S3StorageClient.s3Client plus a single StorageConfig.s3Endpoint, all pointed at one shared MinIO container: S3StorageClientSpec, LargeBinaryManagerSpec, LargeBinaryInputStreamSpec, LargeBinaryOutputStreamSpec. ScalaTest's parallel suite distributor runs them concurrently within a single in-process test task, so they contend for that one container and intermittently time out. The existing Global / concurrentRestrictions += Tags.limit(Tags.Test, 1) only bounds sbt task concurrency, not ScalaTest's in-JVM distributor (CI logs show pool-N-thread-M-ScalaTest-running-...Spec).

Fix — a reusable @NonParallelTest tag.

  • New org.apache.texera.common.tags.NonParallelTest — a Java @TagAnnotation mirroring amber's IntegrationTest. Suites carrying it must not run concurrently with one another.
  • Tag the four MinIO-backed suites.
  • common/workflow-core/build.sbt: Test / testGrouping gives each tagged suite its own forked JVM. sbt runs forked groups one at a time (Tags.ForkedTestGroup limit), so tagged suites never overlap; every untagged suite stays in a single in-process group and keeps running in parallel. The forked working directory is pinned to the repo root to match the in-process cwd.
  • This mirrors the FileService approach for testcontainer suites, but keyed on an opt-in annotation instead of a hard-coded name pattern, so future container-backed suites just add @NonParallelTest.
  • Detection note: sbt discovers ScalaTest suites by subclass fingerprint, not by annotation, so the tag is not visible in definedTests; it is detected reflectively through the test class loader (loadClass does not run static initializers; a load failure falls back to the parallel group).
  • Also bound the concurrent-upload burst in the >1000 objects test from 16 to 4 threads, so a single suite no longer floods the container.

Why not @IntegrationTest? That tag/lane lives only in the amber module (amber/src/test/integration). amber depends on workflow-core, so a workflow-core suite cannot reference it. The established convention for common-module container-backed tests (e.g. file-service's LakeFS/MinIO suites) is to keep them in normal Test scope and isolate them via forked JVMs — which is what this PR does.

Any related issues, documentation, discussions?

Closes #7049

How was this PR tested?

  • sbt reload loads the build cleanly and type-checks the reflective testGrouping (the testLoader/Tests.InProcess/Tests.SubProcess usage); WorkflowCore/Test/forkOptions confirms the forked working directory resolves to the repo root.
  • Full test compile + a live testGrouping run require the JOOQ codegen prerequisite (a database) and Docker (MinIO), neither available in the authoring environment, so the runtime grouping and de-flaking are validated by CI. Since the failure is flaky, reviewers may want to re-run the amber job a couple of times to confirm stability.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8 [1M context])

@github-actions github-actions Bot added dependencies Pull requests that update a dependency file fix common labels Jul 29, 2026
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @kunwp1, @aglinxinyuan
    You can notify them by mentioning @kunwp1, @aglinxinyuan in a comment.

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.26%. Comparing base (f319f04) to head (5228395).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7051   +/-   ##
=========================================
  Coverage     83.26%   83.26%           
  Complexity      225      225           
=========================================
  Files           533      533           
  Lines         30097    30097           
  Branches       3222     3222           
=========================================
  Hits          25059    25059           
  Misses         4233     4233           
  Partials        805      805           
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 76.76% <ø> (ø) Carriedforward from f319f04
amber 86.45% <ø> (ø) Carriedforward from f319f04
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from f319f04
config-service 66.66% <ø> (ø)
file-service 67.21% <ø> (ø) Carriedforward from f319f04
frontend 83.08% <ø> (ø) Carriedforward from f319f04
notebook-migration-service 78.94% <ø> (ø) Carriedforward from f319f04
pyamber 96.34% <ø> (ø) Carriedforward from f319f04
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from f319f04

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 6 worse · ⚪ 7 noise (<±5%) · 0 without baseline

Compared against main 02ae12b benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 452 0.276 21,439/27,982/27,982 us 🔴 +8.5% / 🔴 +77.4%
🟢 bs=100 sw=10 sl=64 936 0.571 104,888/130,442/130,442 us 🟢 -20.9% / 🔴 +21.5%
🔴 bs=1000 sw=10 sl=64 1,096 0.669 896,084/1,103,702/1,103,702 us 🔴 +16.3% / 🟢 -8.6%
Baseline details

Latest main 02ae12b from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 452 tuples/sec 478 tuples/sec 786.12 tuples/sec -5.4% -42.5%
bs=10 sw=10 sl=64 MB/s 0.276 MB/s 0.292 MB/s 0.48 MB/s -5.5% -42.5%
bs=10 sw=10 sl=64 p50 21,439 us 20,540 us 12,305 us +4.4% +74.2%
bs=10 sw=10 sl=64 p95 27,982 us 25,778 us 15,774 us +8.5% +77.4%
bs=10 sw=10 sl=64 p99 27,982 us 25,778 us 18,978 us +8.5% +47.4%
bs=100 sw=10 sl=64 throughput 936 tuples/sec 915 tuples/sec 999.71 tuples/sec +2.3% -6.4%
bs=100 sw=10 sl=64 MB/s 0.571 MB/s 0.559 MB/s 0.61 MB/s +2.1% -6.4%
bs=100 sw=10 sl=64 p50 104,888 us 104,309 us 100,616 us +0.6% +4.2%
bs=100 sw=10 sl=64 p95 130,442 us 164,941 us 107,356 us -20.9% +21.5%
bs=100 sw=10 sl=64 p99 130,442 us 164,941 us 113,255 us -20.9% +15.2%
bs=1000 sw=10 sl=64 throughput 1,096 tuples/sec 1,101 tuples/sec 1,031 tuples/sec -0.5% +6.3%
bs=1000 sw=10 sl=64 MB/s 0.669 MB/s 0.672 MB/s 0.63 MB/s -0.4% +6.3%
bs=1000 sw=10 sl=64 p50 896,084 us 905,854 us 980,328 us -1.1% -8.6%
bs=1000 sw=10 sl=64 p95 1,103,702 us 949,224 us 1,027,528 us +16.3% +7.4%
bs=1000 sw=10 sl=64 p99 1,103,702 us 949,224 us 1,054,298 us +16.3% +4.7%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,442.36,200,128000,452,0.276,21439.46,27981.57,27981.57
1,100,10,64,20,2137.75,2000,1280000,936,0.571,104887.86,130442.23,130442.23
2,1000,10,64,20,18250.46,20000,12800000,1096,0.669,896083.81,1103701.82,1103701.82

@Yicong-Huang
Yicong-Huang force-pushed the fix/7049-serialize-minio-test-suites branch from 6a532c6 to 737387c Compare July 29, 2026 20:48
@Yicong-Huang Yicong-Huang changed the title test(workflow-core): serialize MinIO-backed suites to de-flake S3 tests test(workflow-core): add @NonParallelTest to de-flake shared-MinIO suites Jul 29, 2026
…ites

`S3StorageClientSpec` intermittently failed 4 tests in the `build / amber` CI
job (scope `WorkflowCore / Test / test`). The four suites mixing
`S3StorageTestBase` (`S3StorageClientSpec`, `LargeBinaryManagerSpec`,
`LargeBinaryInputStreamSpec`, `LargeBinaryOutputStreamSpec`) share a JVM-wide
`S3StorageClient.s3Client` and a single `StorageConfig.s3Endpoint` pointed at
one shared MinIO container. ScalaTest's parallel suite distributor runs them
concurrently within a single in-process test task, so they contend for that one
container and intermittently time out. The existing
`Global / concurrentRestrictions += Tags.limit(Tags.Test, 1)` only bounds sbt
task concurrency, not ScalaTest's in-JVM distributor.

Introduce a reusable `@org.apache.texera.common.tags.NonParallelTest` marker
(a Java `@TagAnnotation`, mirroring amber's `IntegrationTest`) and tag the four
suites. `common/workflow-core/build.sbt` reflects over the tagged suites and
gives each its own forked JVM via `Test / testGrouping`; sbt runs forked groups
one at a time (the `Tags.ForkedTestGroup` limit), so tagged suites never overlap
while every untagged suite runs in parallel in a single forked group. sbt
discovers ScalaTest suites by subclass fingerprint (not by annotation), so the
tag is detected reflectively through the test class loader.

Forking is configured exactly like the FileService testcontainer suites
(`Test / fork := true` + per-group `SubProcess`), which is the layout sbt-jacoco
supports for forked coverage. An earlier attempt left `Test / fork := false` and
forked only some groups; that partially-forked layout ran jacoco-instrumented
classes in forked JVMs without the coverage runtime and aborted with
`$jacocoInit` `ClassCastException: Long cannot be cast to [Z`.

Also bound the concurrent-upload burst in the >1000-objects test from 16 to 4
threads so a single suite no longer floods the container.

Closes apache#7049

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@aglinxinyuan aglinxinyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Yicong-Huang
Yicong-Huang added this pull request to the merge queue Jul 29, 2026
Merged via the queue into apache:main with commit f9cface Jul 29, 2026
31 checks passed
@Yicong-Huang
Yicong-Huang deleted the fix/7049-serialize-minio-test-suites branch July 29, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common dependencies Pull requests that update a dependency file fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: S3StorageClientSpec intermittently fails 4 tests in CI (shared MinIO contention)

3 participants