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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
