Repository navigation
Conversation
Code Coverage Report
|
This was referenced Sep 28, 2026
Closed
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
September 28, 2026 19:13
1520adb to
c95bffe
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Sep 28, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
2 of 8 tasks
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
October 13, 2026 21:30
c95bffe to
7ff1b72
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Oct 13, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
October 13, 2026 21:45
7ff1b72 to
3b46c09
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Oct 13, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
- Revert build.yml timeout from 90 to 120 minutes; 90 was too tight given the MySQL lane alone measures ~69 min locally with no margin. - Rename the coreH2Test/coreMySQLTest/corePostgreSQLTest inputs.property key from includeDockerTaggedTests to coreTestIncludesDockerTaggedTests, distinguishing it from the unrelated extensions.extraProperties[includeDockerTaggedTests] flag consumed by root build.gradle.kts's shared test setup. - Drop the hardcoded "()" from BackendTestExtension's getDisplayName(): @testtemplate methods can declare real parameters, so an empty parameter list misrepresented the method's signature in JUnit XML/HTML reports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
…+ BackendLaneCondition The three excludeTags(otherTwoBackends) clauses in registerCoreTestTask allowed two silent failure modes with no build-time check: a class carrying two backend tags was excluded from every lane, and a class missing its restriction tag ran redundantly in all three. Replace them with a single typed source of truth: - DatabaseBackend: the three core database backends. - DatabaseTest: annotation declaring backends() (defaults to all three), meta- annotated with the lane-inclusion tag and @ExtendWith(BackendLaneCondition). - BackendLaneCondition: an ExecutionCondition that reads the lane's gravitino.core.test.backend system property (already published by registerCoreTestTask) and enables/disables the class accordingly, producing a visible, reported skip rather than silent exclusion from Gradle's discovery. - TestDatabaseTestClassification: a build-time reflection check (runs in coreUnitTest, no database, no Docker) asserting no @databasetest(backends={}) and no leftover raw per-backend @tag usages anywhere in core. core/build.gradle.kts's three-way excludeTags table collapses to a single includeTags(coreDatabaseTestTag); backend restriction now lives entirely in test source. Migrated all three existing usage patterns: AbstractEntityStorageTest (parameter-driven multi-backend via storageProvider()/BackendTestSelector), TestJDBCBackend (@TestTemplate-driven via BackendTestExtension), and TestJdbcPartitionStatisticStorageIT's three single-backend @nested classes. Verified via TestBackendLaneCondition (direct unit coverage) and a real :core:coreH2Test run before/after the change: identical 520 executed tests and outcomes: the only delta is 2 classes (14 tests) that were previously excluded from Gradle's own discovery and now appear as an explicit, reported skip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
…abasetest + BackendLaneCondition" This reverts commit efee6a4.
… change 654cfe3 dropped the hardcoded "()" from BackendTestExtension's getDisplayName() but didn't update this test's assertions, leaving coreUnitTest red on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
compare_legacy_partition() (and its "compare-legacy" CLI subcommand) compared a legacy :core:test manifest against the union of the four split-lane manifests, but nothing in .github/workflows/build.yml ever called it - dev/ci/test-shards.sh excludes :core:test from CI entirely, so no legacy manifest is ever produced to compare against. manifest and reconcile, which ARE CI-wired, are untouched. Removes the dead function, its argparse subcommand, its dedicated unit tests, and the legacy/ fixture directory only they consumed. Drops the now-unused "legacy" choice from MANIFEST_LANES. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
The three excludeTags(otherTwoBackends) clauses in registerCoreTestTask excluded a class from every lane if it carried two conflicting backend tags, instead of running it under each tagged backend. Replace the hand-written per-backend cases with one JUnit boolean tag expression built from a backend->tag map: exclude a class only when it is pinned to another backend and not to this one. A class tagged for several backends now runs under each of them; adding a backend is one map entry. Still a discovery-time PostDiscoveryFilter (same as the tags it replaces), so an excluded class never appears in a lane's JUnit XML - core_test_identity.py's foreign-backend-marker check, which is wired into CI, still sees exactly what it did before. Verified against a real :core:coreH2Test run: reproduced the original bug on the old scheme (a double-tagged class silently excluded from every lane), then confirmed the fix runs it correctly with a clean, single-class XML directory and a passing manifest step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
The previous commit unintentionally bundled in a restore of this file to its pre-compare-legacy-removal state (2 failing tests, referencing functions and fixtures bed4a93 already removed) alongside the unrelated core/build.gradle.kts fix. Not the intended change; reverting to bed4a93's version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
…er tag The gatekeeper+modifier scheme (gravitino-core-database-test required for any DB lane, plus optional per-backend modifier tags defaulting to "all backends" when absent) needed a boolean tag expression to reconcile the two dimensions, and let a class desync from the gatekeeper tag specifically - carrying correct modifier tags but missing the gatekeeper meant total invisibility across every lane. Flatten to three peer tags only. Every database test class carries at least one; "applies to all three" means carrying all three tags, explicitly, not carrying none. Lane filtering drops back to plain tag matching, no expression needed: coreUnitTest: excludeTags(h2Tag, mysqlTag, pgTag, dockerTag) coreH2Test: includeTags(h2Tag) coreMySQLTest: includeTags(mysqlTag) corePostgreSQLTest: includeTags(pgTag) Add @AllBackendsTest, a composed annotation that is purely three @tag meta-annotations and nothing else - no @ExtendWith, no execution-time hook, so it stays a discovery-time tag match like the scheme it replaces. TestAllBackendsTest pins the contract directly: it expands to exactly the three backend tags via the same AnnotationSupport lookup JUnit's engine uses at discovery time, and explicitly asserts the annotation carries no execution-time meta-annotation. Migrated all three real usage patterns (parameter-driven multi-backend via storageProvider()/BackendTestSelector, @TestTemplate-driven via BackendTestExtension, and single-backend @nested classes) to the new scheme. Verified against real :core:coreH2Test/:core:coreUnitTest runs: TestJdbcPartitionStatisticStorageIT's per-backend isolation still holds (only the matching nested class's XML is produced), and an @AllBackendsTest class is correctly excluded from coreUnitTest ("No tests found for given includes") and included in coreH2Test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
…eBackend Raw @tag("gravitino-core-h2-test") string literals (and their multi/-mysql/ -postgresql siblings) were typo-prone: a misspelled tag compiles fine and the class just silently never runs in any lane. @AllBackendsTest already solved this for the all-three-backends case; extend the same discovery-time- safe pattern to the individual-backend case. CoreBackend.H2/.MySQL/.PostgreSQL/.All replace both the raw @tag literals and @AllBackendsTest. Flat names (@CoreH2Test etc.) were considered and rejected: they'd collide with existing classes of those exact names (TestJdbcPartitionStatisticStorageIT$H2Test/$MySQLTest/$PostgreSQLTest) - not just confusingly, but as an actual compile error (an unqualified annotation import shadowed by a same-named nested class). Namespacing under CoreBackend makes that collision structurally impossible (CoreBackend.H2 is always qualified) and matches this package's existing "Backend" vocabulary (BackendTestExtension, BackendTestSelector). A parameterized single annotation (@corebackend(backends = {H2, MYSQL})) was also considered and rejected: JUnit's @tag resolution only ever reads an annotation type's own fixed meta-annotations, never a per-usage attribute value, so it would need an execution-time mechanism to interpret - the same category of mistake the already-reverted @DatabaseTest/ExecutionCondition redesign made. Each nested annotation stays a plain composed @tag with no @ExtendWith; TestCoreDatabaseLaneAnnotations pins the contract directly (exact tag expansion via the same AnnotationSupport lookup Jupiter's engine uses at discovery time, inheritance, stacking, and that none of the four carry an execution-time meta-annotation). Also adds `coreTestLaneOf`, a discovery-only local check (./gradlew :core:coreTestLaneOf -PclassName=<FQCN>) that prints which lane(s) a class's tags put it in without running anything, and warns when a Docker-tagged class carries no backend tag (the one gap this annotation redesign doesn't structurally close: forgetting every backend tag, not just one of several). And a build-script comment on coreBackendTestTags spelling out the four lane-membership shapes with examples. Verified against real :core:coreH2Test/:core:coreUnitTest/:core:coreTestLaneOf runs: TestJdbcPartitionStatisticStorageIT's per-backend isolation still holds (only the matching nested class's XML is produced, manifest passes), and coreTestLaneOf correctly reports lanes for a real @CoreBackend.All class and warns for a synthetic Docker-tagged, no-backend-tag orphan class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
:core:test is the java plugin's built-in test task, kept registered for tooling backward-compat but no longer the right way to run core tests. Its tasks.test block now warns on every direct invocation, naming the four real lanes (coreUnitTest/coreH2Test/coreMySQLTest/ corePostgreSQLTest) and coreTestLaneOf. No CI change needed since dev/ci/test-shards.sh already excludes it. Adds design-docs/testing/core-db-split.md, an SPIP (Heilmeier Catechism) documenting the final lane-split design retroactively, including the alternatives that were tried and reverted along the way and a contributor how-to (Appendix C) in place of a docs/how-to-test.md section, since that doc is repo-wide and never mentioned core specifically.
TestCoreDatabaseLaneAnnotations' @ParameterizedTest used the default display name, which embeds each case's expected-tags argument (e.g. "[gravitino-core-h2-test]") into its own JUnit XML. That test runs in coreUnitTest, so core_test_identity.py's unit-lane manifest check rejected it as a foreign backend marker - reproduced directly, confirmed with a full 1968-testcase sweep through the real normalize_identity function after the fix, zero marker errors. Fixed by naming on the class under test only. Also, found via two independent Fable reviews: - CoreBackend's Javadoc misattributed core_test_identity.py's reconcile step to core/build.gradle.kts, and its lane-membership bullets didn't mention the docker-tag-only orphan case. Fixed both. - The SPIP's Appendix C taught a partial-backend tagging example (@CoreBackend.H2 @CoreBackend.MySQL) that fails CI's reconcile step unless normalized sibling classes exist for the omitted backends, which only one existing class shape satisfies. Replaced it with guidance to default to @CoreBackend.All and an explicit warning about the constraint.
A class tagged gravitino-docker-test with no CoreBackend tag is excluded from coreUnitTest and from every database lane, and reconcile only compares the database lanes with each other, so it could drop out of all three at once with CI green. TestCoreLaneMembership runs in coreUnitTest and fails, naming each such class.
The root setTestEnvironment reads the flag inside param.doFirst, at execution time, after every register/configureEach action has run, so the core database lanes keep their Docker-tagged tests regardless of configuration order. Comment both ends so the read stays in doFirst.
Only the build suite excludes :core:test. BackendIT's others shard still realizes it through the root test task under -PskipTests, where it matches nothing in core, so the deprecation warning can appear there.
For a class with no tests of its own, such as TestJdbcPartitionStatisticStorageIT, coreTestLaneOf now reports each member test class and its lanes instead of warning that the class runs nowhere. Non-static @nested classes also pick up their enclosing classes' tags. TestCoreLaneMembership reuses the same helpers.
TestCoreLaneMembership now fails coreUnitTest for a Docker-tagged class with no backend tag, so the doc no longer describes that case as unguarded.
Wire coreUnitTest/coreH2Test/coreMySQLTest/corePostgreSQLTest into a core-test matrix job in build.yml directly, so the PR no longer depends on the Backend IT sharding in apache#13495 (dev/ci/test-shards.sh). The build job skips :core:test and uploads its JaCoCo reports; a coverage job merges them with the four core lane .exec files after core-test-contract reconciles the lanes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019TX7XCnuCMGSJ7xX2iubTH
Drop the design doc, the Python test-identity tool with its tests and fixtures, the core-test-contract job and manifest steps, and the coreTestLaneOf helper task. TestCoreLaneMembership keeps the orphan-lane guard; its two tag helpers move into the test itself.
CoreBackend tags choose the lane; MySQL and PostgreSQL need Docker and H2 does not. gravitino-docker-test on a backend class only keeps it out of coreUnitTest and of legacy :core:test, so an H2-tagged class must not need a container.
Say that an untagged Docker class runs in no lane task (legacy :core:test still runs it with Docker on), say plainly why lane annotations must not carry an execution-time hook, and name what the includeDockerTaggedTests flag actually controls.
Review asked to keep only the core changes, so remove what doesn't earn its keep: - TestCoreDatabaseLaneAnnotations only re-tested JUnit's own @tag meta-annotation and @inherited handling. The lanes themselves and TestCoreLaneMembership already prove the tags select the right tests. - The lane rules were spelled out three times (core/build.gradle.kts, CoreBackend, the :core:test block). CoreBackend's Javadoc is now the one place; the Gradle file points to it. - The coreTestIncludesDockerTaggedTests input property only repeated what the coreTestBackend input already tells Gradle's up-to-date check.
… trivia Review asked to keep only the core changes: - Gradle already writes each Test task's JUnit XML to build/test-results/<task> and its JaCoCo data to build/jacoco/<task>.exec, so the lanes no longer set those paths. The coreTestSuite and coreTestBackend inputs only repeated what includeTags and the backend system property already give the up-to-date check, and the three imports are Kotlin DSL defaults. - validateCoreSuiteCoverage checked that the four .exec files exist, which the coverage job already checks before it copies them. - TestBackendTestSelector no longer pins the six-row legacy matrix or the display-name string through a mocked ExtensionContext; the remaining tests cover selection, validation and the selected-backend matrix.
Contributors had no guide to the new lanes: name the four tasks and when Docker is needed, how a class joins a lane through @corebackend, and how to check a class runs in the lane you expect.
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
October 13, 2026 23:56
3b46c09 to
6587460
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Oct 13, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
apache#13478 added docs/policies.md as an input and gravitino.repositoryRoot as a system property to :core:test. TestRestrictedRegoGoldenFile is a unit test, so CI runs it in coreUnitTest: give that task the same two settings. The database lanes never run it and get neither.
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
October 14, 2026 02:42
6587460 to
3496aa2
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Oct 14, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
…gain CloseContainerExtension runs for every integration-test module. Making it rethrow turned a slow Docker teardown at the end of a green run into a failed task. Keep ContainerSuite.close() throwing for direct callers, but log in the extension as before.
The fork count and the number of lanes on the command line don't change test results, but as task inputs they made every machine and every task combination miss the build cache.
nevzheng
force-pushed
the
codex/13529-core-database-adaptive-forks
branch
from
October 14, 2026 19:28
3496aa2 to
1eae08e
Compare
nevzheng
pushed a commit
to nevzheng/gravitino
that referenced
this pull request
Oct 14, 2026
Stack: apache#13517 (lanes) -> apache#13518 (fixture reuse) -> apache#13530 (adaptive forks, one container per worker) -> this change. apache#13530 gets its wall-clock win from Gradle test-worker forks, but pays for it with one physical MySQL/PostgreSQL container per fork. This adds SharedDbContainerService, a Gradle BuildService that starts one MySQL and one PostgreSQL container for the whole build (via the docker CLI, since the main Gradle process has no Testcontainers on its classpath), and provisions a brand-new, uniquely-named database per DefaultBackendFactory.create() call instead of a fresh container. Falls back to the existing per-fork-container path (BaseIT#startAndInitMySQLBackend()/startAndInitPGBackend()) when the shared-container system properties aren't published, e.g. an IDE run. apache#13530's own DatabaseIsolation/BackendSetupCallback/BackendResource reuse-and-poison caching is untouched - this only changes how a backend is provisioned, not when one is reused. Reviewed twice independently (two blind Fable passes); both converged on the same real issues (stale-container cleanup not covering a killed-daemon leak, a test clobbering a Gradle-owned system property, under-tested PostgreSQL duplicate-database handling) plus additional individual findings, all fixed: - Stale-container cleanup now tracks the owning Gradle daemon's PID and reaps a still-running container only when that process is dead, instead of never reaping a running container at all. - SharedDbContainerService's docker-CLI parsing (host-port parsing, line splitting, PID-liveness check) extracted to static methods and unit-tested without needing Docker. - DatabaseProvisioning's PostgreSQL exists-check/duplicate-tolerant create sequence extracted to Statement-level methods and unit-tested with Mockito (42P04 handling, non-duplicate rethrow). - Fixed a test that cleared org.gradle.test.worker instead of saving and restoring it, matching the sibling telemetry test's pattern. - Documented the 63-char name cap and exception-type change for existing MySQLContainer/PostgreSQLContainer callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015k3E9XykKTzbTWaBPwqTyE
jerryshao
added a commit
that referenced
this pull request
Oct 15, 2026
…es (#13517) ### What changes were proposed in this pull request? Split `core` tests into four Gradle lanes, each run as its own CI job, so unit tests and each database backend report, fail and time independently. - `coreUnitTest` runs tests with no database backend. `coreH2Test`, `coreMySQLTest` and `corePostgreSQLTest` each run one backend, sequentially. A class joins a lane with `@CoreBackend.H2/MySQL/PostgreSQL/All`. - `TestCoreLaneMembership` fails `coreUnitTest` when a Docker-tagged class has no backend tag, so a class can't silently drop out of every lane. - The `coverage` job merges the four lanes' raw JaCoCo data into one core report. - Three plain `@Test` methods move from `TestJobMetaService` to `TestJobMetaServiceValidation`: they need no database, and in a `@CoreBackend.All` class they would run three times. - `:core:test` still works locally and prints a deprecation warning. - `docs/how-to-test.md` describes the lanes and how to check which lane a class runs in. ### Why are the changes needed? `:core:test` combines unit tests with all three database backends in one task. Failures, timing and coverage can't be told apart per backend, and the follow-up concurrency work (#13518, #13530, #13553) has no safe boundary to build on. Running the lanes in parallel also shortens the critical path (see Timing). Fix: #13516 Related to: #13515 ### Does this PR introduce _any_ user-facing change? No production API, storage format or configuration changes. Test-only changes: - New Gradle tasks `coreUnitTest`, `coreH2Test`, `coreMySQLTest`, `corePostgreSQLTest`, and the test-only system property `gravitino.core.test.backend`. - `:core:test` is deprecated: it still runs all core tests locally but prints a warning pointing to the lanes. - Core `@TestTemplate` cases are now reported as `testMethod[H2 Backend]` instead of `[H2 Backend]`, in JUnit XML and HTML for the new lanes and for legacy `:core:test`. Test history keyed on the old names starts fresh, and exact-match `--tests` filters need the new name. Trailing-wildcard filters such as `*[H2 Backend]` still match. - CI checks: `build (17)` and the BackendIT checks keep their names, but `build (17)` now skips `:core:test`, so core results appear only under the new `core-test (17, unit|h2|mysql|postgresql)` checks. `coverage` is now its own check. `.asf.yaml` requires no status checks by name, so branch protection needs no change. Merge order for the follow-up PRs is on #13515. ### How was this patch tested? Locally (macOS, JDK 17, Docker via OrbStack) on the PR head: - `./gradlew :core:spotlessCheck :core:compileTestJava -PskipWeb=true -PskipDockerTests=true` - `./gradlew :core:coreUnitTest -PskipWeb=true -PskipDockerTests=true`: 2,203 tests, 0 failures. - `./gradlew :core:coreH2Test -PskipWeb=true -PskipDockerTests=true`: 588 tests, 0 failures; no Docker container was started. - `./gradlew :core:corePostgreSQLTest -PskipWeb=true -PskipDockerTests=false`: 588 tests, 0 failures, 3 skipped. - `./gradlew :core:coreMySQLTest -PskipWeb=true -PskipDockerTests=false`: 588 tests, 0 failures, 2 skipped. - The three database lanes ran the same 588 tests once backend names are normalized, and no class ran in both the unit lane and a database lane. All 312 core test classes JUnit discovers ran in a lane. - `TestCoreLaneMembership` fails, naming the four `TestEntityStorage*` classes, when `@CoreBackend.All` is removed from `AbstractEntityStorageTest`. - `actionlint .github/workflows/build.yml` <details><summary>Timing (CI)</summary> Lane jobs from [run 38280654558](https://github.com/apache/gravitino/actions/runs/38280654558) on `656282cc6`. Later commits remove code and comments and leave lane selection unchanged; the lanes' JUnit and JaCoCo outputs now use Gradle's default paths. | Job | Wall time | | --- | ---: | | `build (17)` on `main`, core included ([1](https://github.com/apache/gravitino/actions/runs/38357234677), [2](https://github.com/apache/gravitino/actions/runs/38245414857), [3](https://github.com/apache/gravitino/actions/runs/38139898061)) | 1h28m – 1h46m | | `build (17)` on this PR, core skipped | 49m30s | | `core-test (17, unit)` | 6m04s | | `core-test (17, h2)` | 6m59s | | `core-test (17, postgresql)` | 19m11s | | `core-test (17, mysql)` | 27m20s | | `coverage` | 1m28s | The lanes run in parallel with `build`, so the critical path is `build` plus `coverage`, about 51m. </details> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Jerry Shao <jerryshao@datastrato.com>
Collaborator
Author
|
Closing in favour of a smaller stack. With one test JVM per lane, which is what CI's runners already get, #13518 (class reuse plus resetting only non-empty tables) and #13899 (disposable test-database data) take the three core database lanes from about 55 to about 10 minutes, with no fork policy. Thanks for looking! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Let the core H2, MySQL and PostgreSQL lanes use up to two Gradle test JVMs, while JUnit stays serial inside each JVM.
maxParallelForksfrom CPU count, JVM-visible memory, Gradle worker capacity, the number of database lanes selected together, and a rollout cap of 2.DatabaseTestContextby exact type, keeping [#6505] improvement(test): Reuse core database fixtures by test class #13518's class-scoped cleanup and rebuild behaviour.[CORE-DB-SHARD]telemetry, and require it in each database lane's CI evidence.The unit lane and legacy
:core:testare unchanged, and JUnit method-level parallelism stays off.Why are the changes needed?
#13518 gives each test class an explicit fixture lifecycle, but each database lane still ran in one JVM and left CPU unused. Bounded JVM-level concurrency cuts lane time while each JVM keeps its own database server.
Fix: #13529
Depends on: #13518
Related to: #13515
Does this PR introduce any user-facing change?
No production change. New test- and CI-only controls:
coreDatabaseForks/CORE_DATABASE_FORKScoreDatabaseBuildMemoryReserveMiB/CORE_DATABASE_BUILD_MEMORY_RESERVE_MIBcoreDatabaseForkMemoryOverheadMiB/CORE_DATABASE_FORK_MEMORY_OVERHEAD_MIBHow was this patch tested?
./gradlew -p buildSrc testpasses, and./gradlew :core:coreUnitTest :core:coreH2Test -PskipWeb=true -PskipDockerTests=trueran 2,216 unit tests and 588 H2 tests with 0 failures; the H2 lane ran in 2 JVMs.forks=1 limitingFactor=MEMORYfor all three lanes (run 38502451524). So the speed-up below is a local result until the memory budget orCORE_DATABASE_FORKSis adjusted for CI.Matched local runs on macOS (OrbStack, JDK 17, 14 processors, 48 GiB), measured before the rebase:
Stack: #13517 → #13518 → this PR → #13553