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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
