Repository navigation
[#13516] improvement(ci): Split core tests into explicit database lanes - #13517
Conversation
Code Coverage Report
Files
|
jerryshao
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
Verdict: blocking issues
Reviewed 3058fb44945f4df06a2a01e482006462a73f5997 against origin/main (merge base 19e3a555). The branch still carries the #13495 commits, so I concentrated on the #13516 delta: core/build.gradle.kts, the includeDockerTaggedTests hook in root build.gradle.kts, the new backend selector and tags, dev/ci/test-shards.sh, dev/ci/core_test_identity.py, and the new build.yml jobs.
The overall shape is good: lanes are derived from one shard table, the database lanes are forced sequential, JaCoCo exec files are collected per lane and merged once, and the identity reconciliation fails closed. Two things need attention before merge.
Blocking. The includeDockerTaggedTests opt-in is never observed by the root test configuration, because Gradle runs configureEach add-actions before the action passed to tasks.register. :core:coreH2Test therefore keeps the inherited excludeTags("gravitino-docker-test") whenever dockerTest is false, which is exactly how CI runs the core-h2 shard. The H2 lane then loses ~52 AbstractEntityStorageTest cases plus TestJdbcPartitionStatisticStorageIT$H2Test, and core-test-contract fails its H2-vs-MySQL/PostgreSQL identity check. I reproduced the ordering on Gradle 8.2 rather than inferring it; details are in the inline comment on build.gradle.kts.
Tests. Coverage of the new logic is good: TestBackendTestSelector covers the selector's absent/normalized/invalid paths and both provider shapes, and dev/ci/tests/test_core_test_identity.py covers normalization, the unit-lane foreign-marker rejection and reconciliation against committed fixtures. Two gaps:
- Nothing exercises the Gradle wiring itself. The defect above is invisible to every test in this PR and to every local command listed in the description; it only surfaces in the
core-h2CI shard. The real fix is to make each lane's tag set independent of-PskipDockerTests; a cheap extra guard would be acore-test-contractassertion that the H2 manifest contains theTestEntityStorage*identities. compare-legacyis implemented and unit-tested (dev/ci/core_test_identity.py:543-562) but no workflow calls it. I grepped.github/anddev/ci/and found no invocation. The description lists "verifies that legacy execution is the exact multiset union of the split lanes" as a delivered property, while the appendix marks the legacy partition comparison as TBD. Could you either wire acompare-legacystep in (a manually dispatched job would do) or soften that bullet, so the description matches what CI actually enforces?
Findings that could not be anchored inline: none beyond the two notes above.
Review drafted with Claude, requested by @jerryshao.
- 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
|
@jerryshao PTAL Changes:
Makes the bug class unrepresentable rather than just detectable post-execution. Listing (before/after equivalence,
Same 520 executed, same outcomes. The 14 "new" tests were always excluded from Gradle's own discovery (invisible); now they're a reported skip with a reason string — e.g. Also added Nevin |
|
need redesign. will reply later |
|
@jerryshao PTAL — here's what changed since your initial review, and how it's been tested. What changed:
How it's been tested:
|
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
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
Verdict: needs more information
Re-review of the 11 commits since 3058fb44.
Previously raised:
- [Important]
includeDockerTaggedTestsnever observed, so thecore-h2lane silently loses every Docker-tagged test — no longer reproduces on this head. CI has since run (run36466794673):build (17, core-h2)passed with-PskipDockerTests=true,:core:coreH2Testtook 216.8s there against your local 3m40.981s for the full lane, andcore-test-contractpassed — that job enforces strict h2 == mysql == postgresql identity equality (dev/ci/core_test_identity.py:418-424), so the four@CoreBackend.Allclasses did run in the H2 lane. Why the root Docker exclusion did not bite is the one thing I could not establish; see the question oncore/build.gradle.kts:180. - [Question]
timeout-minutes90 → 120 — fixed (.github/workflows/build.yml:162). - [Nit]
inputs.property("includeDockerTaggedTests")colliding with the extraProperties flag — fixed, renamed tocoreTestIncludesDockerTaggedTestswith a comment separating the two mechanisms. - [Nit] hardcoded
()inBackendTestExtension#getDisplayName— fixed (BackendTestExtension.java:145-151). - [Tests]
compare-legacyimplemented but unwired — resolved by removing it (bed4a93), along with its fixture and unit tests;argparsestill requires a subcommand, so theelsebranch inmainis reachable only byreconcile. Two description follow-ups, though: the bullet "verifies that legacy execution is the exact multiset union of the split lanes" is no longer a property of this branch, and "9 tests passed" is now 8 (python3 -B -m unittest discover -s dev/ci/tests -p 'test_core_test_identity.py'→ 8 tests, OK, run here). - [Tests] nothing exercises the lane wiring itself — partly addressed.
TestCoreDatabaseLaneAnnotationspins tag expansion, stacking, inheritance and the absence of any non-@Tagmeta-annotation, andcore-test-contractnow catches divergence between lanes. What is still unguarded is a drop that hits all three lanes equally, or a class that lands in no lane at all — see the comment oncore/build.gradle.kts:174.
The redesign itself reads well: one backend→tag table, includeTags only, no raw lane @Tag strings left in core/src/test, and no execution-time condition (the SPIP records that attempt and why it was reverted).
Tests: adequate for the new logic. TestCoreDatabaseLaneAnnotations covers all four annotations plus stacking, subclass inheritance and the meta-annotation shape; the Python suite passes here; the lane split itself is now backed by a real four-lane CI run. The two gaps above are the remainder: no build-time guard against an unlaned class, and the -PskipDockerTests interaction is unpinned.
Timing: the after-CI column in the description is still "Pending", but run 36466794673 supplies it — core-unit 6m54s, core-h2 6m33s, core-postgresql 17m40s, core-mysql 25m26s of job wall time, against the 49m35s combined-core baseline you cite, i.e. a ~24m critical-path reduction with core-mysql as the long pole. Worth filling in, since it is a stronger claim than the local model.
Findings that could not be anchored inline: only the description items above.
Review drafted with Claude, requested by @jerryshao.
|
@jerryshao PTAL. All four threads from your Sep 29 re-review are addressed, one commit each:
Nevin |
|
I would suggest that we simplify the code. Current PR has lots of code that are not directly related to the PR, like design doc, test fixtures, Python code, and shell scripts. I don't think they're super necessary, it makes others hard to maintain. I would suggest we only keep the core changes. |
jerryshao
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
This is well-engineered, and I want to say so specifically because several things I expected to be problems turned out to be deliberate and correct:
othersemits-x :core:test, and the four new lanes aretasks.register<Test>that are not wired intocheck, so rootbuildgenuinely cannot run them a second time — the comment inprint_othersholds.- The artifact path design is right: mixing
core/build/...andbuild/reports/...in onepath:forces upload-artifact's least-common-ancestor back to the workspace root, so after downloadcore-test-evidence/unit/core/build/jacoco/coreUnitTest.execis exactly whatinstallexpects. Same trick withbuild.gradle.ktsanchoring thejacoco-*artifacts, and it's documented inline. coverage-comment.ymlkeeps working: the coverage steps moved to a separate job but stay in the same workflow run, so theworkflow_runtrigger andrun-idstill resolve.- Skipped tests still emit
<testcase>, so PostgreSQL's "517 passed + 2 skipped" and H2's 519 are identity-equal andreconcilewon't false-alarm. - The
TestJobMetaServiceValidationsplit is legitimate —JobMetaServicethrowsNoSuchEntityExceptionon malformed ids before any DB access, so those three tests really need no backend. CoreTestLaneOf.declaresTestscorrectly excludes abstract classes, andeffectiveTagsonly walks enclosing classes for non-static nested classes, which matches JUnit (a@Nested static classisn't a JUnit nested class and is discovered as a top-level one).
Four comments inline, none blocking. The one I'd most like to see changed is the loose backend-token regex in core_test_identity.py, which is a trap for future contributors rather than a bug today.
Two things not worth their own comment: backend-integration-test-action.yml drops timeout-minutes from 120 to 90 with no backend-it timing evidence in this PR (the timing table only covers the core lanes) — that's #13495's change, but the 25% cut deserves a datapoint there. And TestCoreLaneMembership's findAllClassesInPackage("org.apache.gravitino", ...) classloads every class under that package; JUnit's scanner catches per-class Throwable so it's safe, it just adds a fixed cost to coreUnitTest.
0e66fd6 to
656282c
Compare
- 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
note: Done in 6797513, 11ac69a and 4e8e79d. The PR now keeps only the four |
|
@jerryshao PTAL, and please let me know specifically any blocking issues or asks. Let's get this merged; I have follow-ups (#13518, #13530, #13553) waiting on it. A lot changed since your last review, so here is the summary. Change logMajor
Other
Replies to your comments
All 12 inline threads are resolved. Evidence
CI on |
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
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
…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.
1147573 to
483bee3
Compare
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.
483bee3 to
5481579
Compare
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
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
Verdict: ship it
Re-review after the rebase and the scope cut. My last review was on 0e66fd6d, which the rebase removed from the branch, so there is no usable 0e66fd6d...HEAD range; I reviewed the whole change at 8cc5fbf9 against origin/main (f58ea843) instead, and checked each earlier finding against the current tree.
Previously raised:
- [Oct 8] Loose
BACKEND_TOKEN_REindev/ci/core_test_identity.py- gone. The file is no longer in the tree (git diff --stat origin/main...HEADlists 14 files, none underdev/ci/), so nothing parses testcase names any more. Resolved by removal. - [Oct 8] Check-name cutover and branch protection - resolved, and I verified the claim rather than taking it:
.asf.yaml:48-49hasrequired_status_checks: strict: truewith nocontexts:list, so no check is required by name and INFRA has nothing to reconfigure. The rebase also dropped #13495, sobuild (17)and the BackendIT names are unchanged; the new names arecore-test (17, <lane>)andcoverage. - [Oct 8] h2 lane running with
dockerTest=falsewhile the other two seetrue- the constraint is now recorded inCoreBackend.java:36-38("the lane, not the class, decides whether Docker is needed"). The container-level-conditional edge it protected against left withreconcile. Accepted, but see the question oncore/build.gradle.kts:157, which is the part of this that got less guarded, not more. - [Oct 8]
@TestTemplaterename blast radius - fixed in the description; the user-facing section now spells outtestMethod[H2 Backend], the loss of test history and the--testsfilter change. - [Sep 29] Orphan-lane guard - still in place and still the right shape (
TestCoreLaneMembership), with one residual hole; see the nit on that file. - [Sep 29]
coreTestLaneOfnested-class verdict - obsolete, the task was removed in the cut.docs/how-to-test.md:51-57replaces it with a--testsprobe, which is accurate: Gradle's filter fails on no match by default.
I also re-checked the three tests moved out of TestJobMetaService: JobMetaService.deleteJob calls parseJobRunId first (JobMetaService.java:163-164) and getJobPO parses before touching SessionUtils (:186-193), and parseJobRunId throws NoSuchEntityException on a malformed id, so TestJobMetaServiceValidation really needs no backend. No unused imports are left behind in TestJobMetaService (NoSuchEntityException, JobHandle, NameIdentifierUtil are all still used). Of the 38 direct subclasses of TestJDBCBackend, none still declares a plain @Test method and none has a further subclass, so that cleanup is complete rather than partial.
Tests: adequate for the new logic. TestBackendTestSelector covers absent/normalized/invalid selection and the storageProvider filter; TestCoreLaneMembership covers the orphan-class case. What is still unpinned is the Gradle wiring: nothing fails if the h2 lane stops picking up Docker-tagged classes (the question below), and the orphan guard reads class-level tags only (the nit below).
One caveat on the evidence. The four-lane run you cite, 38280654558, is on 656282cc6, and its job list still contains core-test-contract plus a "Generate core test manifest" step in each lane - i.e. it validates the lane selection, but not the build.yml on this head, which dropped that job and moved the lane outputs to Gradle's default paths. Every job of that run that was not skipped, coverage included, did succeed, and I traced the current paths by hand: core/build/test-results/<task>, core/build/jacoco/<task>.exec (both Gradle defaults) and build/reports/tests/core/<task> (set by root build.gradle.kts:1019-1021) match what "Validate core test evidence" asserts, and the mixed core/... plus build/... upload keeps the artifact anchored at the workspace root so the coverage job's find -path "*/core/build/jacoco/<task>.exec" resolves. Worth a green run on this head before merge, nothing more.
Not worth their own comments: maxParallelForks = 1 and junit.jupiter.execution.parallel.enabled=false (core/build.gradle.kts:160-161) restate today's defaults - there is no junit-platform.properties anywhere in the repo and Gradle's default is one fork - so they are documentation today and load-bearing only once #13518 turns parallelism on; fine either way, I mention it only because the cut removed other default-restating lines. -PskipITs on the lane jobs is a no-op for :core (only the flink/authorization modules read that property), harmless and consistent with the build job.
Findings that could not be anchored inline: none.
Review drafted with Claude, requested by @jerryshao.
|
@jerryshao thanks for the approval! CI is green on your merge commit. Could you shepherd this to merge? The branch needs an update from If you'd like any change before merge, could you list it specifically: the file, what to change, and whether it blocks? I'm otherwise treating your three latest comments as non-blocking. |
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
What changes were proposed in this pull request?
Split
coretests into four Gradle lanes, each run as its own CI job, so unit tests and each database backend report, fail and time independently.coreUnitTestruns tests with no database backend.coreH2Test,coreMySQLTestandcorePostgreSQLTesteach run one backend, sequentially. A class joins a lane with@CoreBackend.H2/MySQL/PostgreSQL/All.TestCoreLaneMembershipfailscoreUnitTestwhen a Docker-tagged class has no backend tag, so a class can't silently drop out of every lane.coveragejob merges the four lanes' raw JaCoCo data into one core report.@Testmethods move fromTestJobMetaServicetoTestJobMetaServiceValidation: they need no database, and in a@CoreBackend.Allclass they would run three times.:core:teststill works locally and prints a deprecation warning.docs/how-to-test.mddescribes the lanes and how to check which lane a class runs in.Why are the changes needed?
:core:testcombines 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:
coreUnitTest,coreH2Test,coreMySQLTest,corePostgreSQLTest, and the test-only system propertygravitino.core.test.backend.:core:testis deprecated: it still runs all core tests locally but prints a warning pointing to the lanes.@TestTemplatecases are now reported astestMethod[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--testsfilters need the new name. Trailing-wildcard filters such as*[H2 Backend]still match.build (17)and the BackendIT checks keep their names, butbuild (17)now skips:core:test, so core results appear only under the newcore-test (17, unit|h2|mysql|postgresql)checks.coverageis now its own check..asf.yamlrequires no status checks by name, so branch protection needs no change. Merge order for the follow-up PRs is on [EPIC] Improve database-backed test structure and execution #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.TestCoreLaneMembershipfails, naming the fourTestEntityStorage*classes, when@CoreBackend.Allis removed fromAbstractEntityStorageTest.actionlint .github/workflows/build.ymlTiming (CI)
Lane jobs from run 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.build (17)onmain, core included (1, 2, 3)build (17)on this PR, core skippedcore-test (17, unit)core-test (17, h2)core-test (17, postgresql)core-test (17, mysql)coverageThe lanes run in parallel with
build, so the critical path isbuildpluscoverage, about 51m.