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/09/22 15:19:20 UTC
[PR] [GLUTEN-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
LuciferYang opened a new pull request, #13100:
URL: https://github.com/apache/gluten/pull/13100
## What changes are proposed in this pull request?
`ShuffleManagerRegistry.register` guards against registering `GlutenShuffleManager` itself, which would make the router route into itself and recurse. The condition is inverted:
```scala
require(
!clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
"It's not allowed to register GlutenShuffleManager recursively")
```
`!clazz.isAssignableFrom(classOf[GlutenShuffleManager])` only rejects `GlutenShuffleManager` and its supertypes; a subclass of `GlutenShuffleManager` passes. Registering one while `spark.shuffle.manager` is the experimental `GlutenShuffleManager` recurses on the first routed shuffle (`registerShuffle` → `getOrBuild` → instantiate → route → …) until the stack overflows. The guard has been inverted since the registry was introduced in #8084, so it has never blocked a subclass.
This adds the missing direction so the guard rejects `GlutenShuffleManager` and any subtype, keeping the supertype case it already covered. Legitimate managers such as `ColumnarShuffleManager` and `SortShuffleManager` are unaffected, since neither is related to `GlutenShuffleManager` by assignability.
## How was this patch tested?
`GlutenShuffleManagerSuite` gains two cases: registering a `GlutenShuffleManager` subclass is now rejected (it was wrongly allowed before), and registering the `ShuffleManager` interface itself is still rejected.
## Was this patch authored or co-authored using generative AI tooling?
Yes. Generated-by: ZCode (GLM-5.3).
Related issue: #13099.
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13100:
URL: https://github.com/apache/gluten/pull/13100#issuecomment-5796251393
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13100:
URL: https://github.com/apache/gluten/pull/13100#discussion_r4082860570
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -32,14 +32,21 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
// will take higher precedence during lookup.
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
- require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ // Check the ShuffleManager implementation first, so a class that is not a
+ // ShuffleManager at all (e.g. a non-ShuffleManager supertype of
+ // GlutenShuffleManager) is rejected with the accurate message rather than
+ // the recursion one.
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
s"$shuffleManagerClass"
)
+ require(
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
Review Comment:
The recursion guard uses a double-negative with an `&&`, which is harder to read and easier to get wrong during future edits. Consider rewriting it in a positive form (e.g., compute a boolean like `isRelatedToGluten = clazz.isAssignableFrom(classOf[GlutenShuffleManager]) || classOf[GlutenShuffleManager].isAssignableFrom(clazz)` and then `require(!isRelatedToGluten, ...)`) to make the intent and truth table clearer.
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -32,14 +32,21 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
// will take higher precedence during lookup.
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
- require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ // Check the ShuffleManager implementation first, so a class that is not a
+ // ShuffleManager at all (e.g. a non-ShuffleManager supertype of
+ // GlutenShuffleManager) is rejected with the accurate message rather than
+ // the recursion one.
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
s"$shuffleManagerClass"
)
+ require(
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
+ "It's not allowed to register GlutenShuffleManager or its subtype / supertype " +
+ "recursively"
Review Comment:
The error message is a bit ambiguous/awkward (\"subtype / supertype\") and doesn’t clearly state what’s being rejected and why. Consider rephrasing to something more explicit, e.g., indicating that registering `GlutenShuffleManager` itself or any subclass/supertype (including `ShuffleManager`) is disallowed to prevent router recursion; optionally include the offending class name to aid debugging.
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13100:
URL: https://github.com/apache/gluten/pull/13100#discussion_r4110033799
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -32,14 +32,21 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
// will take higher precedence during lookup.
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
- require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ // Check the ShuffleManager implementation first, so a class that is not a
+ // ShuffleManager at all (e.g. a non-ShuffleManager supertype of
+ // GlutenShuffleManager) is rejected with the accurate message rather than
+ // the recursion one.
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
s"$shuffleManagerClass"
)
+ require(
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
Review Comment:
Done in 2ee61d039. Extracted a positive boolean `isGlutenShuffleManagerRelated = clazz.isAssignableFrom(classOf[GlutenShuffleManager]) || classOf[GlutenShuffleManager].isAssignableFrom(clazz)` and require `!isGlutenShuffleManagerRelated`, so the truth table reads directly.
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -32,14 +32,21 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
// will take higher precedence during lookup.
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
- require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ // Check the ShuffleManager implementation first, so a class that is not a
+ // ShuffleManager at all (e.g. a non-ShuffleManager supertype of
+ // GlutenShuffleManager) is rejected with the accurate message rather than
+ // the recursion one.
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
s"$shuffleManagerClass"
)
+ require(
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
+ "It's not allowed to register GlutenShuffleManager or its subtype / supertype " +
+ "recursively"
Review Comment:
Done in 2ee61d039. The message now spells out what is rejected and why, and includes the offending class name: "Cannot register GlutenShuffleManager, any of its subclasses, or a supertype such as the ShuffleManager interface, because the router would then recurse into itself. Offending class: $shuffleManagerClass".
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13100:
URL: https://github.com/apache/gluten/pull/13100#issuecomment-5842791113
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13100:
URL: https://github.com/apache/gluten/pull/13100#discussion_r4073461960
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -33,8 +33,11 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
+ "It's not allowed to register GlutenShuffleManager or its subtype / supertype " +
+ "recursively"
+ )
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
Review Comment:
Because the recursion guard runs before the `ShuffleManager`-implementation check, passing a non-`ShuffleManager` class that happens to be a supertype of `GlutenShuffleManager` (e.g., `java.lang.Object`) will fail with the recursion-guard message, which is misleading. Consider reordering/structuring the validation so the `ShuffleManager` requirement is checked first, and/or narrow the recursion message to only the cases that actually cause routing recursion (typically `GlutenShuffleManager` and its subtypes), while keeping a dedicated rejection/message for `ShuffleManager` itself if needed.
##########
gluten-core/src/test/scala/org/apache/spark/shuffle/GlutenShuffleManagerSuite.scala:
##########
@@ -169,6 +169,28 @@ class GlutenShuffleManagerSuite extends SharedSparkSession {
classOf[GlutenShuffleManager].getName))
}
+ test("register recursively - subclass of GlutenShuffleManager") {
+ val registry = ShuffleManagerRegistry.get()
+
+ assertThrows[IllegalArgumentException](
+ registry.register(
+ new LookupKey {
+ override def accepts[K, V, C](dependency: ShuffleDependency[K, V, C]): Boolean = true
+ },
+ classOf[GlutenSubShuffleManager].getName))
+ }
+
+ test("register recursively - ShuffleManager interface") {
+ val registry = ShuffleManagerRegistry.get()
+
+ assertThrows[IllegalArgumentException](
+ registry.register(
+ new LookupKey {
+ override def accepts[K, V, C](dependency: ShuffleDependency[K, V, C]): Boolean = true
+ },
+ classOf[ShuffleManager].getName))
+ }
Review Comment:
These tests repeat the same inline `LookupKey` definition. Extracting a shared `LookupKey` instance (e.g., a private `acceptAllKey`) would reduce duplication and make future additions/changes less error-prone.
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13100:
URL: https://github.com/apache/gluten/pull/13100#issuecomment-5780817726
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13100:
URL: https://github.com/apache/gluten/pull/13100#discussion_r4082712177
##########
gluten-core/src/main/scala/org/apache/spark/shuffle/ShuffleManagerRegistry.scala:
##########
@@ -33,8 +33,11 @@ class ShuffleManagerRegistry private[ShuffleManagerRegistry] {
def register(lookupKey: LookupKey, shuffleManagerClass: String): Unit = {
val clazz = Utils.classForName(shuffleManagerClass)
require(
- !clazz.isAssignableFrom(classOf[GlutenShuffleManager]),
- "It's not allowed to register GlutenShuffleManager recursively")
+ !clazz.isAssignableFrom(classOf[GlutenShuffleManager]) &&
+ !classOf[GlutenShuffleManager].isAssignableFrom(clazz),
+ "It's not allowed to register GlutenShuffleManager or its subtype / supertype " +
+ "recursively"
+ )
require(
classOf[ShuffleManager].isAssignableFrom(clazz),
s"Shuffle manager class to register is not an implementation of Spark ShuffleManager: " +
Review Comment:
Moved the `ShuffleManager` implementation check ahead of the recursion guard, so a class that is not a `ShuffleManager` (e.g. a non-`ShuffleManager` supertype such as `java.lang.Object`) is now rejected with the accurate "not an implementation of Spark ShuffleManager" message. `GlutenShuffleManager`, its subtypes, and the `ShuffleManager` interface itself are still caught by the recursion guard. Fixed in 0b5cd8a96.
##########
gluten-core/src/test/scala/org/apache/spark/shuffle/GlutenShuffleManagerSuite.scala:
##########
@@ -169,6 +169,28 @@ class GlutenShuffleManagerSuite extends SharedSparkSession {
classOf[GlutenShuffleManager].getName))
}
+ test("register recursively - subclass of GlutenShuffleManager") {
+ val registry = ShuffleManagerRegistry.get()
+
+ assertThrows[IllegalArgumentException](
+ registry.register(
+ new LookupKey {
+ override def accepts[K, V, C](dependency: ShuffleDependency[K, V, C]): Boolean = true
+ },
+ classOf[GlutenSubShuffleManager].getName))
+ }
+
+ test("register recursively - ShuffleManager interface") {
+ val registry = ShuffleManagerRegistry.get()
+
+ assertThrows[IllegalArgumentException](
+ registry.register(
+ new LookupKey {
+ override def accepts[K, V, C](dependency: ShuffleDependency[K, V, C]): Boolean = true
+ },
+ classOf[ShuffleManager].getName))
+ }
Review Comment:
The inline `LookupKey` follows the existing pattern throughout this suite (the pre-existing `register recursively` and `register duplicated` cases use the same). Extracting a shared key would either also touch those unrelated pre-existing tests or leave the suite half-refactored, which is out of scope for this one-line guard fix, so I am keeping it consistent with the surrounding style.
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #13100: [GLUTEN-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register
URL: https://github.com/apache/gluten/pull/13100
--
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-13099][CORE] Fix inverted recursion guard in ShuffleManagerRegistry.register [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #13100:
URL: https://github.com/apache/gluten/pull/13100#issuecomment-5957203100
Closing after a second look. The existing guard already rejects every reachable case: GlutenShuffleManager itself and its supertypes (the ShuffleManager interface included) are caught, and the only real callers register ordinary ShuffleManager implementations (ColumnarShuffleManager from the Velox and Bolt listeners, SortShuffleManager at init). The sole gap this closed was registering a subclass of GlutenShuffleManager, which no backend does and which has no instance anywhere in the tree. That is validation hardening for a case that does not arise, not a fix for a reachable bug, so I would rather not carry it.
--
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