[GLUTEN-12655][CORE] Reset the component graph after ComponentSuite - #12658
Conversation
The component graph and the discovery latch are JVM-global. ComponentSuite registers dummy components into the graph, including a deliberate dependency cycle, and never removes them, so any later suite in the same JVM that calls Component#sorted fails with "Cycle detected in the component graph: B, D, C". No suite in gluten-core happens to call it after ComponentSuite today, which is why this has gone unnoticed, but eight call sites in main reach it, including GlutenDriverPlugin#init and GlutenSessionExtensions, so any future suite that boots a SparkContext or builds session extensions hits it. Add a testing-only reset that empties the graph, clears each component's registration flag so it can register again, and unlatches discovery. Call it from ComponentSuite#afterAll. Add ComponentGraphResetSuite as the guard: it registers a cycle, asserts sorting reports it, resets, then asserts sorting succeeds and the dummy components are gone. Verified it fails when the reset body is emptied.
The guard suite only caught an all-or-nothing regression. Deleting the flag
reset in Registry#clear, deleting the latch re-arm, or deleting ComponentSuite's
afterAll override each left gluten-core fully green, because the suite asserted
through sortedUnsafe() and its final assertion was vacuous once the graph was
empty.
Rewrite the suite around two mutants that now fail:
- clear then re-register the same instance, which only works if Registry#clear
resets the registration flag;
- run ComponentSuite in-process and assert the graph is empty afterwards, which
covers the fix's own call site without depending on suite execution order.
The cycle test now asserts isEmpty rather than absence-by-name, and cleanup moved
to afterAll so a failed assertion cannot leak the cycle it registers.
Keep the flag and the graph in sync: ensureRegistered rolls the flag back when
graph.add throws, since a component that never entered the graph is invisible to
Registry#clear and could otherwise never register again. The rollback covers only
graph.add; extending it over dependencies() would roll back a component that is
already in the graph, making the next registration fail with a misleading
"UID already registered".
Make resetRegisteredForTesting final. Nine of the eleven in-repo Component
implementations sit in this package and could otherwise override it to a no-op
and silently defeat the clear.
Narrow the clearAllForTesting scaladoc to what it actually guarantees: values
derived from an earlier Component.sorted() are not reset, and rediscovery
installs fresh instances. Note on the latch line that it exists for the backend
modules, whose classpath carries component files, so it does not read as dead
code in gluten-core.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a JVM-global test isolation problem in gluten-core where ComponentSuite registers dummy components (including a deliberate dependency cycle) into the global component graph and leaves them behind, breaking later suites that call Component.sorted().
Changes:
- Add a testing-only reset (
clearAllForTesting) that clears the component graph and re-arms the discovery latch. - Make
Component.ensureRegistered()roll back its registration flag ifgraph.add()fails, and ensure graph clearing resets per-component registration flags. - Add
ComponentGraphResetSuiteto validate the reset behavior and thatComponentSuitecleans up inafterAll.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gluten-core/src/test/scala/org/apache/gluten/component/ComponentSuite.scala | Clears the JVM-global component graph in afterAll to avoid leaking dummy components into later suites. |
| gluten-core/src/test/scala/org/apache/gluten/component/ComponentGraphResetSuite.scala | Adds regression tests proving the graph reset works and that ComponentSuite leaves the graph empty. |
| gluten-core/src/main/scala/org/apache/gluten/component/package.scala | Introduces clearAllForTesting() to clear the graph and re-arm component discovery. |
| gluten-core/src/main/scala/org/apache/gluten/component/Component.scala | Adds graph-clearing hooks and registration-flag reset logic to support test isolation and re-registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Catching Throwable also matches VirtualMachineError and friends, where rolling back a registration flag serves no purpose. graph.add only throws IllegalArgumentException from its own require checks, so NonFatal covers every case the rollback is meant for.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
gluten-core/src/test/scala/org/apache/gluten/component/ComponentGraphResetSuite.scala:94
- FailureCollectingReporter mutates a shared mutable.Buffer without any synchronization. ScalaTest can deliver Reporter events from different threads (e.g., when suites/tests are run concurrently), which can lead to races/corruption and flaky failures when the nested ComponentSuite is run.
private class FailureCollectingReporter extends Reporter {
private val buffer = mutable.Buffer[String]()
def failures: Seq[String] = buffer.toSeq
gluten-core/src/main/scala/org/apache/gluten/component/package.scala:59
- Scaladoc grammar is a bit confusing here: "Neither are the per-vertex ... flags" is missing the verb (e.g., "reset"). Rewording makes the behavior and non-reset guarantees clearer.
* Only the graph and the latch are reset. Values derived from an earlier [[Component.sorted]] are
* not: `BackendsApiManager.backend`, `GlutenCostModel.costModelRegistry` and the `graphCache`
* inside `Transition.factory` keep what they computed from the pre-clear component set. Neither
* are the per-vertex `TransitionGraph.Vertex.initialized` flags, so a re-registered component
* does not add its transition edges a second time. Rediscovery also constructs fresh component
|
cc @jackylee-ch |
|
Thank you @zhztheplayer @jackylee-ch |
What changes were proposed in this pull request?
ComponentSuiteregisters dummy components into the JVM-global component graph, including a deliberate dependency cycle, and never removes them. Any later suite in the same JVM that callsComponent.sorted()then fails withCycle detected in the component graph: B, D, C, naming components unrelated to the failing test. Nothing ingluten-corecallssorted()afterComponentSuitetoday, but ten call sites in main sources reach it, so a suite that boots aSparkContextwith the Gluten plugin fails as soon as it is ordered afterComponentSuite.Component's graph and theallComponentsLoadeddiscovery latch areobject-level state with no reset, so a suite that registers components cannot clean up after itself. This adds a testing-only reset that empties the graph, clears each component's registration flag so it can register again, and re-arms the latch.ComponentSuite#afterAllcalls it.ensureRegisterednow rolls its flag back whengraph.addthrows. A component that never entered the graph is invisible toRegistry#clear, so without the rollback its flag stays set and it can never register again. The rollback covers onlygraph.add, notdependencies(): a component that is already in the graph would then fail its next registration with a misleading "UID already registered".resetRegisteredForTestingisfinal, since nine of the eleven in-repoComponentimplementations sit in this package and could otherwise override it to a no-op and silently defeat the clear.Four values survive the reset, and the
clearAllForTestingscaladoc now says so:BackendsApiManager.backend,GlutenCostModel.costModelRegistry, thegraphCacheinsideTransition.factory, and the per-vertexTransitionGraph.Vertex.initializedflags all keep what they computed from the pre-clear component set. Rediscovery also constructs fresh instances, so one of those values can end up holding an instance that is no longer the one in the graph. The latch line carries a comment explaining it serves the backend modules, whose classpath carries component files, so it does not read as dead code here.How was this patch tested?
Added
ComponentGraphResetSuitewith three tests, each covering one part of the reset:ComponentSuitein-process vianew ComponentSuite().run(None, Args(reporter))and assert the graph is empty afterwards. That covers the fix's own call site without depending on suite execution order, and fails whenComponentSuite#afterAllis removed.Cleanup lives in
afterAllso a failed assertion cannot leak the cycle the suite registers. I verified each test fails against the corresponding mutant, and thatmvn -pl gluten-core,gluten-substrait testpasses (37 + 51), both in the natural suite order and withComponentSuiteforced to run first.Closes #12655