Skip to content

[MINOR] Split ITConversionController so its tests can use the fork pool - #883

Closed
slachiewicz wants to merge 1 commit into
apache:mainfrom
slachiewicz:split-it-conversion-controller
Closed

[MINOR] Split ITConversionController so its tests can use the fork pool#883
slachiewicz wants to merge 1 commit into
apache:mainfrom
slachiewicz:split-it-conversion-controller

Conversation

@slachiewicz

Copy link
Copy Markdown
Member

What is the purpose of the pull request

ITConversionController is the single longest-running thing in CI, and because of how failsafe is configured it cannot be shortened by any amount of extra hardware.

The root pom sets reuseForks=false alongside forkCount=6. That means a test class occupies exactly one fork and its tests run serially inside it — forkCount parallelises across classes, never within one. ITConversionController held 13 test methods expanding to 44 invocations, and in a recent run it reported:

Tests run: 44, ... Time elapsed: 1003 s -- in org.apache.xtable.ITConversionController

For context, in that same run XTable Project Core took 18:05 of a 27:22 total, and the next-longest test classes are well below the 1003s mark (ITDeltaConversionSource 684s, ITHudiConversionSource 635s). So this one class sets the floor for the whole build: more forks, a bigger runner, or build caching cannot move it.

Brief change log

  • Add ConversionControllerTestBase, an abstract class holding the shared Spark fixture (@BeforeAll / @AfterAll), the checkDatasetEquivalence overloads, the UUID comparison helper, the partition argument builders and getTableSyncConfig
  • Split the tests across five classes, grouped by what they exercise:
Class Methods Invocations
ITConversionController testVariousOperations 16
ITConversionControllerIncrementalSync time travel, single-format, out-of-sync, no-op, retention 8
ITConversionControllerSchemaEvolution UUID columns, Delta column mapping, corrupted-snapshot recovery 6
ITConversionControllerConcurrentWrites concurrent inserts, table-service/compaction 8
ITConversionControllerPartitioning partition transforms, other Iceberg partition types 6

Test bodies and assertions are unchanged — this is a move, not a rewrite. 13 methods and 44 invocations before and after.

The base class is named ConversionControllerTestBase on purpose: it matches neither failsafe's **/IT*.java includes nor surefire's **/*Test.java patterns, so it is not collected as a test class in its own right.

Each class gets its own JVM and therefore its own SparkSession. That costs a little startup per class, which is the trade being made for letting the existing forks run the groups concurrently.

Verify this pull request

This pull request is already covered by existing tests — it is a reorganisation of them.

Verified locally on Temurin 11 with the project's wrapper:

  • ./mvnw -pl xtable-core verify -Dit.test='ITConversionController*' reports Tests run: 44, the same count as the class before the split
  • The failsafe output now shows all five classes starting concurrently, where previously there was a single Running org.apache.xtable.ITConversionController line with everything serialised behind it
  • spotless:check and apache-rat:check pass

Expected effect, and its limit

I want to be precise rather than optimistic here. testVariousOperations is roughly 64% of the original class's test time, so after the split the largest piece lands around 640s. That is just below ITDeltaConversionSource at 684s, which then becomes the binding constraint for xtable-core.

So the expected shape is xtable-core roughly 18:05 → ~11:30, and the total build roughly 27:22 → ~21:00. Splitting testVariousOperations any further would not help until ITDeltaConversionSource and ITHudiConversionSource are split too — which is why this PR stops where it does.

Those are model-based numbers from per-class timings, not a measurement; the CI run on this PR is the real check, and I am happy to post the observed XTable Project Core time here once it completes.

Failsafe is configured with reuseForks=false, so a test class occupies
exactly one fork and its tests run serially there no matter how high
forkCount is. ITConversionController held 13 test methods expanding to
44 invocations, and at 1003s it was the single longest thing in the
build - xtable-core takes 18:05 of a 27:22 CI run, and the class alone
accounts for most of that. No amount of extra forks, caching or runner
capacity can shorten it while it is one class.

Move the shared Spark fixture and the assertion helpers into a new
abstract ConversionControllerTestBase and split the tests across five
classes grouped by what they exercise:

  ITConversionController                    testVariousOperations
  ITConversionControllerIncrementalSync     time travel, single-format,
                                            out-of-sync, no-op, retention
  ITConversionControllerSchemaEvolution     UUID columns, Delta column
                                            mapping, snapshot recovery
  ITConversionControllerConcurrentWrites    concurrent inserts, compaction
  ITConversionControllerPartitioning        partition transforms

Test bodies and assertions are unchanged; 13 methods and 44 invocations
before and after. The base class is deliberately named so that it
matches neither failsafe's IT* includes nor surefire's *Test patterns.

Each class gets its own JVM and therefore its own SparkSession, which
costs a little startup per class but lets the existing forks run the
groups concurrently.
@slachiewicz

Copy link
Copy Markdown
Member Author

Closing this — the CI run on this PR shows it makes the build slower, not faster, so the premise behind it is wrong. Leaving the measurement here since the result is useful on its own.

Maven CI Build hit the workflow's timeout-minutes: 30 at 30m16s, against 27m22s on the run I took as the baseline.

before after
Total integration-test time 3823s 5842s (+53%)
The 44 ITConversionController invocations 1003s 2637s (2.6x)

The reduced ITConversionController, holding only testVariousOperations — 16 of the original 44 invocations — took 1133s by itself, more than the whole 44-invocation class did before the split.

The decisive part is what happened to classes this PR does not touch at all:

ITDeltaConversionSource        684s -> 1002s  (1.46x)
ITHudiConversionSource         635s ->  830s  (1.31x)
ITDeltaKernelConversionSource  456s ->  556s  (1.22x)
ITParquetConversionSource      237s ->  269s  (1.13x)

Untouched tests do not get 20-46% slower because of a refactor elsewhere. That is resource contention.

What I got wrong

I read ITConversionController's 1003s as evidence of a serialization bottleneck: failsafe runs with reuseForks=false, so one class occupies one fork and its tests run serially there regardless of forkCount. That part is true, but the conclusion only follows if the fork pool has spare CPU to absorb more concurrency — and it does not. forkCount=6 already oversubscribes the runner, and every fork is a Spark JVM at -Xmx1500m. Splitting into five classes added four more concurrent Spark JVMs and four more SparkSession startups to a machine that was already saturated, so everything slowed down, including the other tests.

xtable-core looks CPU-bound on the runner rather than serialization-bound. A long class time can be a symptom of contention rather than proof of a bottleneck, and I should have checked for spare capacity before assuming otherwise.

Possibly worth trying instead

If the runner really is oversubscribed, the useful direction is the opposite of this PR: lowering forkCount toward the runner's core count, so six Spark JVMs stop thrashing over four vCPUs. That is a small change, though it needs measuring rather than assuming — I would not want to repeat the mistake in this PR by asserting it.

Otherwise the levers that reduce total work rather than rearranging it: a larger runner if ASF infra offers one for this project, or #882, which lets unchanged modules skip their build and tests entirely.

@slachiewicz slachiewicz closed this Aug 5, 2026
@slachiewicz
slachiewicz deleted the split-it-conversion-controller branch August 5, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant