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