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: [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]
