test(arrowload): share default BVT and fanout cluster - #28648
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed bdb7dc6 against main 44b7f90. No concrete merge-blocking issue found; COMMENT only for this self-authored PR.
The readiness predicate matches the lock client’s raw-inventory owner lookup: every expected CN must be visible with a non-empty lock endpoint in every expected CN’s refreshed local view. It checks all owners, not merely inventory size or frontend connectivity. The shared 30-second context is passed through lookup, authoritative refresh and raw reads; the built-in refresh gate is context-aware. Polling has no detached worker, and ticker/context cleanup is deferred. Missing membership, empty endpoints and refresh errors eventually fail the test with diagnostics rather than skip it. Startup cleanup is registered before readiness can FailNow, and explicit restart paths repeat the barrier before opening new SQL connections.
This is a membership-convergence barrier, not proof of live endpoint reachability or generation-fence recovery. That distinction is appropriate here: it does not clear inactive-owner state, suppress LOAD errors, or retry a failed LOAD. A dead/stale endpoint or a genuine transaction failure can still fail the existing public SQL assertions.
Fixture consolidation preserves the fan-out LOAD and its row-count/distinct-ID/range assertions, moves it into a separate database and separately cleaned connection, and keeps restart last. Sequential subtests do not introduce concurrent access to the shared fixture. Coverage does change from separate default 1-CN and 2-CN lifecycles to a default 2-CN lifecycle; this is not identical topology coverage, although no Arrow assertion/helper is removed. The checked-in release references now use the nested fan-out name.
Optional follow-up: focused readiness-predicate tests for a missing second owner, empty endpoint and refresh timeout would make future changes easier to validate. Also keep a dedicated 1-CN default-path smoke case if that deployment topology is a release requirement. Neither is a demonstrated blocker for this change.
Evidence: complete six-file diff, current discussion/reviews, exact-head cluster helper, raw owner lookup/context-aware cluster APIs, fan-out oracle and restart ordering inspected; head/base rechecked immediately before submission. The reported full-package, focused race and repeated BVT results are author evidence, not independently rerun here. No CI wait or new native/MinIO test execution.
cfbd490 to
8e7798f
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed 8e7798f against main b922526, including the prior review and current five-file change. No concrete new blocker found; COMMENT only for this self-authored PR.
The fanout scenario still executes the same public parallel LOAD and asserts total rows, distinct IDs and ID range. It now runs sequentially as TestArrowLoadBVT/DistributedRecordBatchFanout, with its own database, sql.DB and temporary fixture; its subtest cleanup runs before later BVT cases. No t.Parallel or new goroutine/retry is introduced. The parent BVT connection remains on arrow_bvt, and complete cluster restart remains last, closes the original connection, opens a fresh connection and proves both committed-data persistence and another LOAD in the restarted generation.
Registering cleanup for a nonnil cluster before the startup assertion is appropriate: StartTestCluster can return retained ownership after a cleanup error. Close errors remain test failures rather than being ignored. Existing cluster service/port/admission cleanup is reused; this PR does not change those production/test-framework mechanisms.
The owner-readiness workaround from the earlier iteration is not in this change. The current tree contains the production absence-as-unknown handling, so the tests exercise startup without masking LOAD errors through retries or an artificial inventory barrier.
The earlier coverage caveat remains: separate default 1-CN and 2-CN fixtures become one default 2-CN fixture. All moved scenario assertions remain, but that is not identical topology coverage. A dedicated default 1-CN smoke case remains an optional follow-up if that topology is a release requirement. The fanout oracle proves data correctness on a 2-CN cluster, not independently that both CNs executed work; this limitation predates the move. Checked-in selectors now reference the nested subtest.
Resource benefit is one fewer cluster lifecycle, not more test parallelism. The reported local timing improvement is plausibly startup amortization, not proof of whole-CI speedup. Existing unbounded SQL/close waits and the reported separate close-error/admission-retention timeout are not fixed or newly introduced here.
Evidence/limits: current full diff, discussions/prior review, exact-head cluster setup/Close, fanout helper, BVT ordering and restart path inspected; head/base rechecked before posting. Author-reported full-package, race and same-process repetition results were not independently rerun. No CI wait, native test run or MinIO execution; the existing missing-MinIO skip is not additional coverage provided by this PR.
What type of PR is this?
Which issue(s) this PR fixes:
Related: #26519
What this PR does / why we need it:
Arrow's default BVT and distributed fanout tests previously started separate embedded clusters with the same default configuration. Run their existing assertions in one 2-CN BVT cluster, using a separate database and session for fanout and keeping restart persistence last. This removes one cluster lifecycle without adding parallel tests or changing production behavior.
Rebased onto main at cd8e8d9. Remove this PR's earlier owner-readiness polling workaround because the production owner-discovery fix is now on main through #28649. Register cleanup for a nonnil cluster before asserting startup success, and update the documented test selectors.
Validation
-count=2): passed, 35.165s total.golangci-linton the Arrow package: 0 issues.-race: passed, 92.297s.-race -count=1: passed, 33.383s (adaptive 30s budget selected one repetition from the measured 22.35s BVT).On this host with runtime temporary data on NVMe, the main-version separate fixtures took 32.080s combined; the consolidated BVT took 17.66s and 17.32s in two sequential repetitions. These are local samples, not a prediction of whole-CI savings. The initial baseline attempt failed because the host's
/tmpwas full; its result is excluded from timing comparison, and both sides were rerun using disk-backed temporary data.The existing LocalMinIO subtest skipped because the external
miniobinary is unavailable. This test-only change does not repair the separate close-error/admission-retention failure observed in the previous CI run and does not claim to resolve its 70-minute global timeout.