You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by ho...@apache.org on 2026/07/31 10:54:38 UTC
(gluten) branch main updated: [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite (#12658)
This is an automated email from the ASF dual-hosted git repository.
zhztheplayer pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gluten.git
The following commit(s) were added to refs/heads/main by this push:
new 4d0ba0d201 [GLUTEN-12655][CORE] Reset the component graph after ComponentSuite (#12658)
4d0ba0d201 is described below
commit 4d0ba0d2016de2f5ae7324bbae878e7e367cc382
Author: YangJie <ya...@baidu.com>
AuthorDate: Fri Jul 31 18:54:31 2026 +0800
[GLUTEN-12655][CORE] Reset the component graph after ComponentSuite (#12658)
* fix: stop ComponentSuite leaking its dummy components into later suites
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.
* fix: make the component-graph reset guard cover what it resets
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.
* fix: narrow the registration rollback to NonFatal
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.
---
.../org/apache/gluten/component/Component.scala | 34 ++++++-
.../org/apache/gluten/component/package.scala | 23 +++++
.../component/ComponentGraphResetSuite.scala | 113 +++++++++++++++++++++
.../apache/gluten/component/ComponentSuite.scala | 8 ++
4 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/gluten-core/src/main/scala/org/apache/gluten/component/Component.scala b/gluten-core/src/main/scala/org/apache/gluten/component/Component.scala
index 7e688d6e70..e2256c01e0 100644
--- a/gluten-core/src/main/scala/org/apache/gluten/component/Component.scala
+++ b/gluten-core/src/main/scala/org/apache/gluten/component/Component.scala
@@ -28,6 +28,7 @@ import org.apache.spark.internal.Logging
import java.util.concurrent.atomic.{AtomicBoolean, AtomicInteger}
import scala.collection.mutable
+import scala.util.control.NonFatal
/**
* The base API to inject user-defined logic to Gluten. To register a component, the implementation
@@ -47,10 +48,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 NonFatal(t) =>
+ isRegistered.set(false)
+ throw t
+ }
dependencies().foreach(req => graph.declareDependency(this, req))
}
+ // Visible for testing. Paired with Graph#clear so a cleared component can register again.
+ final private[component] def resetRegisteredForTesting(): Unit = {
+ isRegistered.set(false)
+ }
+
/**
* Determines whether a component should be registered based on runtime conditions. For instance,
* if a component depends on a Spark extension's JAR, this method should be overridden to check
@@ -124,6 +138,12 @@ object Component extends Logging {
graph.sorted()
}
+ // Visible for testing. Internal to component.clearAllForTesting; does not touch the discovery
+ // latch.
+ private[component] def clearForTesting(): Unit = {
+ graph.clear()
+ }
+
private class Registry {
private val lookupByUid: mutable.Map[Int, Component] = mutable.Map()
private val lookupByClass: mutable.Map[Class[_ <: Component], Component] = mutable.Map()
@@ -139,6 +159,12 @@ object Component extends Logging {
lookupByClass(clazz) = comp
}
+ def clear(): Unit = synchronized {
+ lookupByUid.values.foreach(_.resetRegisteredForTesting())
+ lookupByUid.clear()
+ lookupByClass.clear()
+ }
+
def isUidRegistered(uid: Int): Boolean = synchronized {
lookupByUid.contains(uid)
}
@@ -189,6 +215,12 @@ object Component extends Logging {
sortedComponents = None
}
+ def clear(): Unit = synchronized {
+ registry.clear()
+ uidAndDependencyPairs.clear()
+ sortedComponents = None
+ }
+
private def newLookup(): Map[Int, Node] = {
val uidToNodeLookup: mutable.Map[Int, Node] = mutable.Map()
diff --git a/gluten-core/src/main/scala/org/apache/gluten/component/package.scala b/gluten-core/src/main/scala/org/apache/gluten/component/package.scala
index cf0181c39c..9cad0c0406 100644
--- a/gluten-core/src/main/scala/org/apache/gluten/component/package.scala
+++ b/gluten-core/src/main/scala/org/apache/gluten/component/package.scala
@@ -45,4 +45,27 @@ package object component extends Logging {
)
logInfo(s"Components registered within order: ${components.map(_.name()).mkString(", ")}")
}
+
+ /**
+ * Empties the component graph and re-arms the discovery latch, so the next call to
+ * [[Component.sorted]] runs classpath discovery again.
+ *
+ * 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
+ * instances, so such a value can end up holding an instance that is no longer the one in the
+ * graph.
+ *
+ * Visible for testing. The graph and the latch are both JVM-global, so a suite that registers
+ * components of its own leaks them into every later suite in the same JVM. Such a suite must call
+ * this when it finishes.
+ */
+ private[gluten] def clearAllForTesting(): Unit = {
+ Component.clearForTesting()
+ // Re-arms discovery. Unobservable in gluten-core, whose test classpath carries no
+ // 'META-INF/gluten-components' file; the backend modules are the ones that need it.
+ allComponentsLoaded.set(false)
+ }
}
diff --git a/gluten-core/src/test/scala/org/apache/gluten/component/ComponentGraphResetSuite.scala b/gluten-core/src/test/scala/org/apache/gluten/component/ComponentGraphResetSuite.scala
new file mode 100644
index 0000000000..109f01906a
--- /dev/null
+++ b/gluten-core/src/test/scala/org/apache/gluten/component/ComponentGraphResetSuite.scala
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.component
+
+import org.apache.gluten.extension.injector.Injector
+
+import org.scalatest.{Args, BeforeAndAfterAll, Reporter}
+import org.scalatest.events.{Event, SuiteAborted, TestFailed}
+import org.scalatest.funsuite.AnyFunSuite
+
+import scala.collection.mutable
+
+class ComponentGraphResetSuite extends AnyFunSuite with BeforeAndAfterAll {
+ import ComponentGraphResetSuite._
+
+ override protected def afterAll(): Unit = {
+ // Every test here registers components of its own into the JVM-global graph. Clearing from
+ // afterAll rather than from the test body makes sure a failed assertion cannot leak them.
+ clearAllForTesting()
+ super.afterAll()
+ }
+
+ test("a cycle registered by an earlier suite does not outlive the reset") {
+ // Reproduces what ComponentSuite leaves behind: components wired into a cycle. Without the
+ // reset these stay in the JVM-global graph and every later Component#sorted call throws.
+ clearAllForTesting()
+ new CycleA().ensureRegistered()
+ new CycleB().ensureRegistered()
+ assertThrows[UnsupportedOperationException](Component.sortedUnsafe())
+
+ clearAllForTesting()
+
+ // The graph is empty again, so sorting no longer reports the cycle.
+ assert(Component.sortedUnsafe().isEmpty)
+ }
+
+ test("a component cleared from the graph can be registered again") {
+ // Covers the registration-flag reset the clear performs: without it this very instance could
+ // never re-enter the graph, because ensureRegistered short-circuits on its own flag.
+ clearAllForTesting()
+ val standalone = new Standalone()
+ standalone.ensureRegistered()
+ assert(Component.sortedUnsafe().exists(_.name() == StandaloneName))
+
+ clearAllForTesting()
+ assert(Component.sortedUnsafe().isEmpty)
+
+ standalone.ensureRegistered()
+ assert(Component.sortedUnsafe().exists(_.name() == StandaloneName))
+ }
+
+ test("ComponentSuite leaves the component graph empty") {
+ // Covers ComponentSuite's own cleanup without depending on suite execution order: run that
+ // suite in-process, then assert it took its dummy components back out of the graph.
+ clearAllForTesting()
+ val reporter = new FailureCollectingReporter()
+ val status = new ComponentSuite().run(None, Args(reporter))
+ assert(status.succeeds(), s"ComponentSuite itself failed: ${reporter.failures.mkString("; ")}")
+ assert(Component.sortedUnsafe().isEmpty)
+ }
+}
+
+object ComponentGraphResetSuite {
+ private val StandaloneName: String = "reset-standalone"
+
+ /**
+ * Keeps the nested suite's events out of the test log, while retaining enough of them to say what
+ * went wrong if the nested suite itself fails.
+ */
+ private class FailureCollectingReporter extends Reporter {
+ private val buffer = mutable.Buffer[String]()
+
+ def failures: Seq[String] = buffer.toSeq
+
+ override def apply(event: Event): Unit = event match {
+ case e: TestFailed => buffer += s"${e.testName}: ${e.message}"
+ case e: SuiteAborted => buffer += s"${e.suiteName} aborted: ${e.message}"
+ case _ =>
+ }
+ }
+
+ private class Standalone extends Component {
+ override def name(): String = StandaloneName
+ override def dependencies(): Seq[Class[_ <: Component]] = Nil
+ override def injectRules(injector: Injector): Unit = {}
+ }
+
+ private class CycleA extends Component {
+ override def name(): String = "reset-A"
+ override def dependencies(): Seq[Class[_ <: Component]] = Seq(classOf[CycleB])
+ override def injectRules(injector: Injector): Unit = {}
+ }
+
+ private class CycleB extends Component {
+ override def name(): String = "reset-B"
+ override def dependencies(): Seq[Class[_ <: Component]] = Seq(classOf[CycleA])
+ override def injectRules(injector: Injector): Unit = {}
+ }
+}
diff --git a/gluten-core/src/test/scala/org/apache/gluten/component/ComponentSuite.scala b/gluten-core/src/test/scala/org/apache/gluten/component/ComponentSuite.scala
index b062c58b53..1e4335e942 100644
--- a/gluten-core/src/test/scala/org/apache/gluten/component/ComponentSuite.scala
+++ b/gluten-core/src/test/scala/org/apache/gluten/component/ComponentSuite.scala
@@ -27,6 +27,14 @@ import scala.collection.mutable
class ComponentSuite extends AnyFunSuite with BeforeAndAfterAll {
import ComponentSuite._
+ override protected def afterAll(): Unit = {
+ // The component graph is JVM-global, and these tests register dummy components into it,
+ // including a deliberate dependency cycle. Leaving them behind makes any later suite that
+ // calls Component#sorted fail.
+ clearAllForTesting()
+ super.afterAll()
+ }
+
test("Load order") {
val a = new DummyBackend("A") {}
val b = new DummyBackend("B") {}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org