You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/07/30 06:05:24 UTC
[PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
LuciferYang opened a new pull request, #12658:
URL: https://github.com/apache/gluten/pull/12658
### What changes were proposed in this pull request?
`ComponentSuite` registers 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 calls `Component.sorted()` then fails with `Cycle detected in the component graph: B, D, C`, naming components unrelated to the failing test. Nothing in `gluten-core` calls `sorted()` after `ComponentSuite` today, but ten call sites in main sources reach it, so a suite that boots a `SparkContext` with the Gluten plugin fails as soon as it is ordered after `ComponentSuite`.
`Component`'s graph and the `allComponentsLoaded` discovery latch are `object`-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#afterAll` calls it.
`ensureRegistered` now rolls its flag back when `graph.add` throws. A component that never entered the graph is invisible to `Registry#clear`, so without the rollback its flag stays set and it can never register again. The rollback covers only `graph.add`, not `dependencies()`: a component that is already in the graph would then fail its next registration with a misleading "UID already registered". `resetRegisteredForTesting` is `final`, since 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.
Four values survive the reset, and the `clearAllForTesting` scaladoc now says so: `BackendsApiManager.backend`, `GlutenCostModel.costModelRegistry`, the `graphCache` inside `Transition.factory`, and the per-vertex `TransitionGraph.Vertex.initialized` flags 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 `ComponentGraphResetSuite` with three tests, each covering one part of the reset:
- register a cycle, reset, assert the graph is empty;
- register a component, reset, register the same instance again, assert it is back in the graph. This fails without the registration-flag reset;
- run `ComponentSuite` in-process via `new 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 when `ComponentSuite#afterAll` is removed.
Cleanup lives in `afterAll` so a failed assertion cannot leak the cycle the suite registers. I verified each test fails against the corresponding mutant, and that `mvn -pl gluten-core,gluten-substrait test` passes (37 + 51), both in the natural suite order and with `ComponentSuite` forced to run first.
Closes #12655
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #12658:
URL: https://github.com/apache/gluten/pull/12658#issuecomment-5142172201
Thank you @zhztheplayer @jackylee-ch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer merged PR #12658:
URL: https://github.com/apache/gluten/pull/12658
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #12658:
URL: https://github.com/apache/gluten/pull/12658#discussion_r3680421994
##########
gluten-core/src/main/scala/org/apache/gluten/component/Component.scala:
##########
@@ -47,10 +47,23 @@ trait Component {
if (!isRegistered.compareAndSet(false, true)) {
return
}
- graph.add(this)
+ try {
+ graph.add(this)
+ } catch {
+ // Nothing entered the graph, so Graph#clear will not see this component and cannot reset
+ // its flag. Roll the flag back here to keep it in sync with the graph.
+ case t: Throwable =>
+ isRegistered.set(false)
+ throw t
+ }
Review Comment:
Fixed in db5f076. `graph.add` only throws `IllegalArgumentException` from its own `require` checks, so `NonFatal` covers every case the rollback is meant for, and rolling back a registration flag on a `VirtualMachineError` would serve no purpose anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12658:
URL: https://github.com/apache/gluten/pull/12658#issuecomment-5127210451
Run Gluten Clickhouse CI on x86
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #12658:
URL: https://github.com/apache/gluten/pull/12658#issuecomment-5130415613
cc @jackylee-ch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12658:
URL: https://github.com/apache/gluten/pull/12658#issuecomment-5127292955
Run Gluten Clickhouse CI on x86
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12658:
URL: https://github.com/apache/gluten/pull/12658#discussion_r3680383194
##########
gluten-core/src/main/scala/org/apache/gluten/component/Component.scala:
##########
@@ -47,10 +47,23 @@ trait Component {
if (!isRegistered.compareAndSet(false, true)) {
return
}
- graph.add(this)
+ try {
+ graph.add(this)
+ } catch {
+ // Nothing entered the graph, so Graph#clear will not see this component and cannot reset
+ // its flag. Roll the flag back here to keep it in sync with the graph.
+ case t: Throwable =>
+ isRegistered.set(false)
+ throw t
+ }
Review Comment:
Catching `Throwable` here is overly broad (it also matches fatal errors like `VirtualMachineError`). Since the intent is to roll back the registration flag only for normal failures from `graph.add`, prefer catching `scala.util.control.NonFatal` and rethrowing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on PR #12658:
URL: https://github.com/apache/gluten/pull/12658#issuecomment-5140705746
cc @zhztheplayer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org