[SPARK-45762][CORE] Support shuffle managers defined in user jars by changing startup order #43627

abellina · 2023-11-01T16:53:22Z

What changes were proposed in this pull request?

As reported here https://issues.apache.org/jira/browse/SPARK-45762, ShuffleManager instances defined in a user jar cannot be used in all cases, unless specified in the extraClassPath. We would like to avoid adding extra configurations if this instance is already included in a jar passed via --jars.

Proposed changes:

Refactor code so we initialize the ShuffleManager later, after jars have been localized. This is especially necessary in the executor, where we would need to move this initialization until after the replClassLoader is updated with jars passed in --jars.

Before this change, the ShuffleManager is instantiated at SparkEnv creation. Having to instantiate the ShuffleManager this early doesn't work, because user jars have not been localized in all scenarios, and we will fail to load the ShuffleManager defined in --jars. We propose moving the ShuffleManager instantiation to SparkContext on the driver, and Executor.

Why are the changes needed?

This is not a new API but a change of startup order. The changed are needed to improve the user experience for the user by reducing extra configurations depending on how a spark application is launched.

Does this PR introduce any user-facing change?

Yes, but it's backwards compatible. Users no longer need to specify a ShuffleManager jar in extraClassPath, but they are able to if they desire.

This change is not binary compatible with Spark 3.5.0 (see MIMA comments below). I have added a rule to MimaExcludes to handle it 970bff4

How was this patch tested?

Added a unit test showing that a test ShuffleManager is available after --jars are passed, but not without (using local-cluster mode).

Tested manually with standalone mode, local-cluster mode, yarn client and cluster mode, k8s.

Was this patch authored or co-authored using generative AI tooling?

No

abellina · 2023-11-01T16:59:14Z

I'll have to figure out the CI. It seems my fork is running things, but I am getting some failures in this page (AppVeyor and the Notify test workflow)

abellina · 2023-11-01T17:59:56Z

The MIMA tests are failing due to:

[error] spark-core: Failed binary compatibility check against org.apache.spark:spark-core_2.13:3.5.0! Found 1 potential problems (filtered 3908)
[error]  * method this(java.lang.String,org.apache.spark.rpc.RpcEnv,org.apache.spark.serializer.Serializer,org.apache.spark.serializer.Serializer,org.apache.spark.serializer.SerializerManager,org.apache.spark.MapOutputTracker,org.apache.spark.shuffle.ShuffleManager,org.apache.spark.broadcast.BroadcastManager,org.apache.spark.storage.BlockManager,org.apache.spark.SecurityManager,org.apache.spark.metrics.MetricsSystem,org.apache.spark.memory.MemoryManager,org.apache.spark.scheduler.OutputCommitCoordinator,org.apache.spark.SparkConf)Unit in class org.apache.spark.SparkEnv does not have a correspondent in current version

Which makes sense, since I changed SparkEnv.

I am not entirely sure if adding this to MimaExcludes is the right approach here, and I think I need some help.

abellina · 2023-11-01T19:11:52Z

Ok I believe given the mima code that I need to add a temporary skip here: https://github.com/apache/spark/blob/master/project/MimaExcludes.scala#L37.

abellina · 2023-11-01T21:39:07Z

@tgravescs fyi

tgravescs

Overall looks fine, it does complicate the initialization a bit but not sure I see a better way to handle that. Would be good to get more eyes on it.

cc @mridulm since I think you looked at shuffle related stuff in past.

tgravescs · 2023-11-02T14:10:29Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

@@ -71,6 +69,9 @@ class SparkEnv (
    val outputCommitCoordinator: OutputCommitCoordinator,
    val conf: SparkConf) extends Logging {

+  // We set the ShuffleManager in SparkContext and Executor


nit update comment to say something like: the ShuffleManager is initialized later in... to allow it being defined in user specified jars.

tgravescs · 2023-11-02T14:37:17Z

core/src/test/scala/org/apache/spark/storage/BlockManagerReplicationSuite.scala

-        new LiveListenerBus(conf), None, blockManagerInfo, mapOutputTracker, sc.env.shuffleManager,
-        isDriver = true)),
+        new LiveListenerBus(conf), None, blockManagerInfo, mapOutputTracker,
+        sc.env.shuffleManager.shuffleBlockResolver.getBlocksForShuffle, true)),


put back the isDriver = true as last parameter

tgravescs · 2023-11-02T14:44:17Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

    val blockTransferService: BlockTransferService,
    securityManager: SecurityManager,
    externalBlockStoreClient: Option[ExternalBlockStoreClient])
  extends BlockDataManager with BlockEvictionHandler with Logging {

+  // this is set after the ShuffleManager is instantiated in SparkContext and Executor
+  private var shuffleManager: ShuffleManager = _


update description above to mention having to set the shuffle manager as well.

core/src/main/scala/org/apache/spark/SparkEnv.scala

… in BM replication suite

abellina · 2023-11-02T16:10:23Z

@tgravescs thanks for the review. I have handled your comments in this commit: 0bd7e99

mridulm

Given there is a way for users to specify this reasonably right now, the amount of change to add support for this looks a bit lot.
Thoughts @tgravescs ?

mridulm · 2023-11-03T05:12:08Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

@@ -71,6 +69,10 @@ class SparkEnv (
    val outputCommitCoordinator: OutputCommitCoordinator,
    val conf: SparkConf) extends Logging {

+  // We initialize the ShuffleManager later, in SparkContext and Executor, to allow
+  // user jars to define custom ShuffleManagers.
+  var shuffleManager: ShuffleManager = _


Given SparkEnv is a DeveloperApi, let us not expose this for mutation.

Suggested change

var shuffleManager: ShuffleManager = _

private var _shuffleManager: ShuffleManager = _

def shuffleManager: ShuffleManager = _shuffleManager

Will fix !!

abellina · 2023-11-04T21:47:40Z

@mridulm thanks for the comments. I have published a SPIP here https://issues.apache.org/jira/browse/SPARK-45792 that aims to show the bigger picture. Without the change of initialization order in this PR, we couldn't carry out the SPIP linked, because the ShuffleManager is initialized really early in the Executors today. I split this up into a separate PR to not introduce too much change at once, but your point is well taken. I would like to hear your thoughts around the SPIP and how we can proceed.

Note there is an alternative I can easily try and that is to instantiate a ShuffleManager wrapper, which would remove the change to the SparkEnv (we would instantiate the wrapper instead of the actual impl). We could then set the impl on this wrapper at a later time, when jars are localized and plugins are loaded. This felt a bit worse than the approach I have in this PR, but I am happy to hear opinions.

Thanks again!!

beliefer · 2023-11-05T11:50:58Z

core/src/main/scala/org/apache/spark/shuffle/ShuffleManager.scala

+/**
+ * Utility companion object to create a ShuffleManager given a spark configuration.
+ */
+private[spark] object ShuffleManager {


Shall we put the companion object at the last?

beliefer · 2023-11-05T12:02:30Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

@@ -402,7 +405,7 @@ object SparkEnv extends Logging {
            None
          }, blockManagerInfo,
          mapOutputTracker.asInstanceOf[MapOutputTrackerMaster],
-          shuffleManager,
+          shuffleBlockGetterFn,


Why not define shuffleBlockGetterFn in BlockManagerMasterEndpoint?

I see this being an issue in tests where the SparkEnv would not be set, so now I'd have to make sure that the env is set and cleared in the tests. That said, if you feel strongly about this, I can look at this more.

mridulm · 2023-11-06T01:41:47Z

While I am not opposed to a way to create a short name for shuffle manager, if it results in nontrivial changes to Spark, I am not very inclined towards it.
IMO this should be something that is better handled in context of SPARK-25299 and aligned with it - unfortunately that SPIP is partly done.

abellina · 2023-11-06T14:22:04Z

@beliefer thanks for the comments, I handled most of your comments in the last commit (except for the one about the function passing, but we can discuss that one more there).

tgravescs · 2023-11-06T20:47:07Z

I agree that ideally we would finish SPARK-25299, I don't see that happening anytime soon. I also don't think it covers the case of people replacing the entire ShuffleManager vs just the storage piece. ShuffleManager API isn't public either but we have multiple implementations doing that now (Ubers RSS, project Gluten, Spark Rapids, I thought Cosco was although its not open source, etc).
One note is that issue SPARK-25299 had a sub issue that was going to use the SparkPlugin for configuration https://issues.apache.org/jira/browse/SPARK-30033/https://github.com/apache/spark/pull/26670 and had a pr that mentions the weird interaction with initialization and it works around it in a different way.

Overall while there are a bunch of changes here most of it is just moving initialization stuff around that shouldn't impact anything else. The one thing that is user impacting is the SparkEnv api change, which if we only do with 4.0 shouldn't be a big deal, unless there is some usage I'm not aware of. @mridulm Is there a specific part you are concerned with?

abellina · 2023-11-06T22:30:51Z

@tgravescs @mridulm @beliefer I made a small tweak where the executorEnvs map in the SparkContext is populated with the configuration prefix spark.executorEnv.* after the driver plugin is instantiated (see the last two commits).

mridulm · 2023-11-07T06:19:49Z

@tgravescs The SparkEnv related changes, shuffleBlockGetterFn, etc is what gave me pause - SparkEnv create is a bit fragile given the initialization dependencies ... I am less concerned about the Executor side of things.
Given this can be done currently with a couple of configs, it is not very clear to me what the value of making this change is - how bad a pain point it is.

mridulm

I am yet to test my comments, but you can reference a version with the changes here
(SparkSubmitSuite passes).

mridulm · 2023-11-08T05:27:43Z

core/src/main/scala/org/apache/spark/SparkContext.scala

@@ -627,6 +631,7 @@ class SparkContext(config: SparkConf) extends Logging {
    }
    _ui.foreach(_.setAppId(_applicationId))
    _env.blockManager.initialize(_applicationId)
+    _env.blockManager.setShuffleManager(shuffleManager)
    FallbackStorage.registerBlockManagerIfNeeded(_env.blockManager.master, _conf)


We only need _env.initiailzeShuffleManager() (to replace env.setShuffleManager) in this class - we can revert the rest.

mridulm · 2023-11-08T05:33:43Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

+  private[spark] def setShuffleManager(shuffleManager: ShuffleManager): Unit = {
+    _shuffleManager = shuffleManager
+  }


Instead of setting it, expose an initialize method.
We use initializeShuffleManager in driver/executor after classpath has been fixed up (see more below in comment for shuffleBlockGetterFn).

Suggested change

private[spark] def setShuffleManager(shuffleManager: ShuffleManager): Unit = {

_shuffleManager = shuffleManager

}

private[spark] def initiailzeShuffleManager(): Unit = {

Preconditions.checkState(null == _shuffleManager,

"Shuffle manager already initialized to %s", _shuffleManager)

_shuffleManager = ShuffleManager.create(conf, executorId == SparkContext.DRIVER_IDENTIFIER)

}

mridulm · 2023-11-08T05:34:03Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

+    val env = SparkEnv.get
+    env.shuffleManager.shuffleBlockResolver.getBlocksForShuffle(shuffleId, mapId)
+  }
+


Drop this ?
In BlockManagerMasterEndpoint:

We change constructor to:
private val _shuffleManager: ShuffleManager,

And add a field:
private lazy val shuffleManager = Option(_shuffleManager).getOrElse(SparkEnv.get.shuffleManager)

Do the same for BlockManager as well.
See more below in create.

Looking at this later, preserving this is mainly to minimize test code changes, and allow for a way to override it.

mridulm · 2023-11-08T06:00:30Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

-      shortShuffleMgrNames.getOrElse(shuffleMgrName.toLowerCase(Locale.ROOT), shuffleMgrName)
-    val shuffleManager = Utils.instantiateSerializerOrShuffleManager[ShuffleManager](
-      shuffleMgrClass, conf, isDriver)
-
    val memoryManager: MemoryManager = UnifiedMemoryManager(conf, numUsableCores)


~~Instead, do:~~
~~val shuffleManager: ShuffleManager = if (isDriver) ShuffleManager.create(conf, true) else null~~
~~and keep rest of this method the same.~~

Simply pass shuffleManager = null

mridulm · 2023-11-08T06:02:22Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+  private val shuffleManager =
+    Utils.withContextClassLoader(defaultSessionState.replClassLoader) {
+      ShuffleManager.create(conf, true)
+    }
+
+  env.setShuffleManager(shuffleManager)
+  env.blockManager.setShuffleManager(shuffleManager)


Suggested change

private val shuffleManager =

Utils.withContextClassLoader(defaultSessionState.replClassLoader) {

ShuffleManager.create(conf, true)

}

env.setShuffleManager(shuffleManager)

env.blockManager.setShuffleManager(shuffleManager)

if (! isLocal) {

Utils.withContextClassLoader(defaultSessionState.replClassLoader) {

env.initiailzeShuffleManager()

}

}

I have not tested this, but I think this should work. If it does not, most of my suggestions will need to be discarded :-)

mridulm · 2023-11-08T06:16:46Z

core/src/test/scala/org/apache/spark/storage/BlockManagerReplicationSuite.scala

@@ -81,9 +81,10 @@ trait BlockManagerReplicationBehavior extends SparkFunSuite
      conf, securityMgr, serializerManager, "localhost", "localhost", 0, 1)
    val memManager = memoryManager.getOrElse(UnifiedMemoryManager(conf, numCores = 1))


With the proposed changes, we can revert all changes to this file

mridulm · 2023-11-08T06:18:24Z

core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala

@@ -143,10 +143,11 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with PrivateMethodTe
      None
    }


Same as with BlockManagerReplicationSuite, all changes can be reverted here as well.

mridulm · 2023-11-08T06:19:18Z

streaming/src/test/scala/org/apache/spark/streaming/ReceivedBlockHandlerSuite.scala

@@ -93,7 +93,8 @@ abstract class BaseReceivedBlockHandlerSuite(enableEncryption: Boolean)
    val blockManagerInfo = new mutable.HashMap[BlockManagerId, BlockManagerInfo]()
    blockManagerMaster = new BlockManagerMaster(rpcEnv.setupEndpoint("blockmanager",


Here as well, revert all changes.

mridulm · 2023-11-08T08:18:42Z

@abellina, given SPARK-45792 is an SPIP, can you please surface in spark-dev@ and initiate a discussion on it ? I dont remember seeing it there.

abellina · 2023-11-15T20:53:50Z

Thanks @mridulm, yes the commits make sense, it brings back the late initialization in the driver. I tested the change, the main difference from your patch @mridulm is I had to still get the shuffle manage class names using the method we added to the ShuffleManager object here https://github.com/apache/spark/pull/43627/files#diff-42a673b8fa5f2b999371dc97a5de7ebd2c2ec19447353d39efb7e8ebc012fe32R592, because the shuffleManager is not set yet at this point.

@tgravescs fyi

tgravescs · 2023-11-15T21:36:33Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

@@ -71,6 +70,12 @@ class SparkEnv (
    val outputCommitCoordinator: OutputCommitCoordinator,
    val conf: SparkConf) extends Logging {

+  // We initialize the ShuffleManager later in SparkContext, and Executor, to allow


Suggested change

// We initialize the ShuffleManager later in SparkContext, and Executor, to allow

// We initialize the ShuffleManager later in SparkContext and Executor to allow

tgravescs · 2023-11-15T21:40:15Z

core/src/main/scala/org/apache/spark/SparkEnv.scala

+    //     SPARK-45762 introduces a change where the ShuffleManager is initialized later
+    //     in the SparkContext and Executor, to allow for custom ShuffleManagers defined
+    //     in user jars. In the executor, the BlockManager uses a lazy val to obtain the
+    //     shuffleManager from the SparkEnv. In the driver, the SparkEnv's shuffleManager


I think this comment it no longer true. Driver SparkEnv shufflemanager is created after the plugin initialized.

Thanks @tgravescs. Handled both comments here: 6d002a3

…_manager_initialization_order

abellina · 2023-11-16T15:50:32Z

There were some CI failures around missing dependencies in the documentation build (all tests are passing otherwise). So I have upmerged. I also tweaked a couple of comments here: 5480faa

tgravescs

This looks good to me.

mridulm · 2023-11-17T03:07:33Z

Merged to master.
Thanks for working on this @abellina !
Thanks for the reviews @tgravescs, @beliefer :-)

tgravescs · 2023-11-17T14:21:46Z

Thanks @mridulm @abellina

abellina · 2023-11-17T14:30:36Z

Thanks @mridulm @tgravescs and @beliefer for the reviews and rework!

dongjoon-hyun

Hi, @abellina , @mridulm , @tgravescs, @beliefer .

Although this is a developer API, this is a documented one. Do you think we can avoid this breaking change by adding a new constructor instead?

https://spark.apache.org/docs/3.5.0/api/java/org/apache/spark/SparkEnv.html

mridulm · 2024-02-07T07:58:00Z

@dongjoon-hyun, it is @DeveloperApi from point of view of usage - SparkEnv is not expected to be created by users, as some of the constructor parameters are not externally visible (RpcEnv, for example, cannot be created as it is private[spark]). There have been changes to its constructor in the past as well, after it was marked @DeveloperApi - though to be fair, these were a while back.

In general, I am conflicted about trying to preserve compatibility for things which are clearly private to spark - it inhibits the ability for the project to evolve: especially around major version boundaries (though we do have few of these instances where we try to maintain compatibility).

Given how long SparkEnv has been around, I can see case being made for adding a constructor which preserves earlier signature. Thoughts @tgravescs ?

dongjoon-hyun · 2024-02-07T08:49:11Z

@mridulm . Of course, it's legit if it's not easy or there is no other way. Also, we have a similar breaking proposal, #45052 , too. While reviewing that PR, I double-checked this PR briefly.

I'm totally fine if this is inevitable here and there. :)

mridulm · 2024-02-10T08:33:32Z

Thanks for understanding @dongjoon-hyun !

…plugin is loaded ### What changes were proposed in this pull request? This changes the initialization of `SparkEnv.memoryManager` to after the `DriverPlugin` is loaded, to allow the plugin to customize memory related configurations. A minor fix has been made to `Task` to make sure that it uses the same `BlockManager` through out the task execution. Previous a different `BlockManager` could be used in some corner cases. Also added a test for the fix. ### Why are the changes needed? Today, there is no way for a custom `DriverPlugin` to override memory configurations such as `spark.executor.memory`, `spark.executor.memoryOverhead`, `spark.memory.offheap.size` etc This is because the memory manager is initialized before `DriverPlugin` is loaded. A similar change has been made to `shuffleManager` in #43627. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Also added new tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #45052 from sunchao/SPARK-46947. Authored-by: Chao Sun <sunchao@apache.org> Signed-off-by: Chao Sun <sunchao@apache.org>

…plugin is loaded ### What changes were proposed in this pull request? This changes the initialization of `SparkEnv.memoryManager` to after the `DriverPlugin` is loaded, to allow the plugin to customize memory related configurations. A minor fix has been made to `Task` to make sure that it uses the same `BlockManager` through out the task execution. Previous a different `BlockManager` could be used in some corner cases. Also added a test for the fix. ### Why are the changes needed? Today, there is no way for a custom `DriverPlugin` to override memory configurations such as `spark.executor.memory`, `spark.executor.memoryOverhead`, `spark.memory.offheap.size` etc This is because the memory manager is initialized before `DriverPlugin` is loaded. A similar change has been made to `shuffleManager` in apache#43627. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Also added new tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#45052 from sunchao/SPARK-46947. Authored-by: Chao Sun <sunchao@apache.org> Signed-off-by: Chao Sun <sunchao@apache.org>

…tor.userClassPathFirst=true with ShuffleManager defined in user jar ### What changes were proposed in this pull request? `SparkShuffleManager` print warning log for `spark.executor.userClassPathFirst=true` with `ShuffleManager` defined in user jar via `--jar` or `spark.jars`. ### Why are the changes needed? When `spark.executor.userClassPathFirst` is enabled with ShuffleManager defined in user jar, the `ClassLoader` of `handle` is `ChildFirstURLClassLoader`, which is different from `CelebornShuffleHandle` of which the `ClassLoader` is `AppClassLoader` in `SparkShuffleManager#getWriter/getReader`. The local test log is as follows: ``` ./bin/spark-sql --master yarn --deploy-mode client \ --conf spark.celeborn.master.endpoints=localhost:9099 \ --conf spark.executor.userClassPathFirst=true \ --conf spark.jars=/tmp/celeborn-client-spark-3-shaded_2.12-0.5.0-SNAPSHOT.jar \ --conf spark.shuffle.manager=org.apache.spark.shuffle.celeborn.SparkShuffleManager \ --conf spark.shuffle.service.enabled=false ./bin/spark-sql --master yarn --deploy-mode client --jars /tmp/celeborn-client-spark-3-shaded_2.12-0.5.0-SNAPSHOT.jar \ --conf spark.celeborn.master.endpoints=localhost:9099 \ --conf spark.executor.userClassPathFirst=true \ --conf spark.shuffle.manager=org.apache.spark.shuffle.celeborn.SparkShuffleManager \ --conf spark.shuffle.service.enabled=false ``` ``` 24/04/28 18:03:31 [Executor task launch worker for task 0.0 in stage 5.0 (TID 8)] WARN SparkShuffleManager: [getWriter] handle classloader: org.apache.spark.util.ChildFirstURLClassLoader, CelebornShuffleHandle classloader: sun.misc.Launcher$AppClassLoader ``` It causes that `SparkShuffleManager` fallback to vanilla Spark `SortShuffleManager` for `spark.executor.userClassPathFirst=true` with `ShuffleManager` defined in user jar before apache/spark#43627. After [SPARK-45762](https://issues.apache.org/jira/browse/SPARK-45762), the `ClassLoader` of `handle` and `CelebornShuffleHandle` are both `ChildFirstURLClassLoader`. ``` 24/04/28 18:03:31 [Executor task launch worker for task 0.0 in stage 5.0 (TID 8)] WARN SparkShuffleManager: [getWriter] handle classloader: org.apache.spark.util.ChildFirstURLClassLoader, CelebornShuffleHandle classloader: org.apache.spark.util.ChildFirstURLClassLoader ``` Therefore, `SparkShuffleManager` should print warning log to remind for `spark.executor.userClassPathFirst=true` with `ShuffleManager` defined in user jar. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual test. Closes #2482 from SteNicholas/CELEBORN-1402. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: SteNicholas <programgeek@163.com>

Support shuffle managers defined in user jars by changing startup order

b39da83

github-actions bot added CORE DSTREAM labels Nov 1, 2023

abellina changed the title ~~SPARK-45762: Support shuffle managers defined in user jars by changing startup order~~ [SPARK-45762][CORE]: Support shuffle managers defined in user jars by changing startup order Nov 1, 2023

HyukjinKwon changed the title ~~[SPARK-45762][CORE]: Support shuffle managers defined in user jars by changing startup order~~ [SPARK-45762][CORE] Support shuffle managers defined in user jars by changing startup order Nov 2, 2023

abellina added 2 commits November 2, 2023 08:28

Fix minor typo in SparkSubmitSuite

82bd134

Add exclude for SparkEnv.this in MimaExcludes

970bff4

github-actions bot added the BUILD label Nov 2, 2023

Upmerge

c3c6905

tgravescs reviewed Nov 2, 2023

View reviewed changes

Code review handling: added comments and brought back isDriver = true…

0bd7e99

… in BM replication suite

mridulm reviewed Nov 3, 2023

View reviewed changes

beliefer reviewed Nov 5, 2023

View reviewed changes

Update for review comments

eec52a4

abellina added 2 commits November 6, 2023 16:03

get executorEnv after the driver plugin starts

79d1b99

Move the create call closer to _env.setShuffleManager

03f158a

mridulm reviewed Nov 8, 2023

View reviewed changes

mridulm referenced this pull request Nov 8, 2023

Alternative to validate comments, and to provide reference for PR 43627

d5ba0cc

abellina added 2 commits November 15, 2023 12:34

Initialize shuffle manager from SparkContext

41dc012

Adjust comments and access for SparkEnv method

ea1e165

tgravescs reviewed Nov 15, 2023

View reviewed changes

abellina added 3 commits November 15, 2023 15:56

Update comments

6d002a3

Improve comment in block manager and the master endpoint

5480faa

Merge branch 'master' of https://github.com/apache/spark into shuffle…

6a3f958

…_manager_initialization_order

tgravescs approved these changes Nov 16, 2023

View reviewed changes

mridulm approved these changes Nov 17, 2023

View reviewed changes

mridulm closed this in 7c146c9 Nov 17, 2023

gerashegalov mentioned this pull request Nov 21, 2023

[FEA] Explore javaagent API for loading shims and config simplification NVIDIA/spark-rapids#3803

Closed

gerashegalov mentioned this pull request Jan 11, 2024

Add tests that use Spark's --packages to fetch the plugin NVIDIA/spark-rapids#10160

Closed

sunchao mentioned this pull request Feb 6, 2024

[SPARK-46947][CORE] Delay memory manager initialization until Driver plugin is loaded #45052

Closed

dongjoon-hyun reviewed Feb 6, 2024

View reviewed changes

SteNicholas mentioned this pull request Apr 28, 2024

[CELEBORN-1402] SparkShuffleManager print warning log for spark.executor.userClassPathFirst=true with ShuffleManager defined in user jar apache/celeborn#2482

Closed

ulysses-you mentioned this pull request May 29, 2024

[CORE][SPARK-4.0] Only require one config spark.plugins to enable gluten apache/incubator-gluten#5915

Open

gerashegalov mentioned this pull request Jun 28, 2024

[BUG] Rework RapidsShuffleManager initialization for Apache Spark 4.0.0 NVIDIA/spark-rapids#11107

Closed

mridulm mentioned this pull request Sep 5, 2024

[SPARK-49502][CORE] Avoid NPE in SparkEnv.get.shuffleManager.unregisterShuffle #47977

Closed

-  var shuffleManager: ShuffleManager = _
+  private var _shuffleManager: ShuffleManager = _
+  def shuffleManager: ShuffleManager = _shuffleManager

-  private[spark] def setShuffleManager(shuffleManager: ShuffleManager): Unit = {
-    _shuffleManager = shuffleManager
-  }
+  private[spark] def initiailzeShuffleManager(): Unit = {
+    Preconditions.checkState(null == _shuffleManager,
+      "Shuffle manager already initialized to %s", _shuffleManager)
+    _shuffleManager = ShuffleManager.create(conf, executorId == SparkContext.DRIVER_IDENTIFIER)
+  }

		@@ -81,9 +81,10 @@ trait BlockManagerReplicationBehavior extends SparkFunSuite
		conf, securityMgr, serializerManager, "localhost", "localhost", 0, 1)
		val memManager = memoryManager.getOrElse(UnifiedMemoryManager(conf, numCores = 1))

		@@ -143,10 +143,11 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with PrivateMethodTe
		None
		}

		@@ -93,7 +93,8 @@ abstract class BaseReceivedBlockHandlerSuite(enableEncryption: Boolean)
		val blockManagerInfo = new mutable.HashMap[BlockManagerId, BlockManagerInfo]()
		blockManagerMaster = new BlockManagerMaster(rpcEnv.setupEndpoint("blockmanager",

	// We initialize the ShuffleManager later in SparkContext, and Executor, to allow
	// We initialize the ShuffleManager later in SparkContext and Executor to allow

[SPARK-45762][CORE] Support shuffle managers defined in user jars by changing startup order #43627

[SPARK-45762][CORE] Support shuffle managers defined in user jars by changing startup order #43627

Conversation

abellina commented Nov 1, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

abellina commented Nov 1, 2023

abellina commented Nov 1, 2023

abellina commented Nov 1, 2023 • edited Loading

abellina commented Nov 1, 2023

tgravescs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina commented Nov 2, 2023

mridulm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm commented Nov 6, 2023 • edited Loading

abellina commented Nov 6, 2023

tgravescs commented Nov 6, 2023

abellina commented Nov 6, 2023

mridulm commented Nov 7, 2023 • edited Loading

mridulm left a comment • edited Loading

Choose a reason for hiding this comment

mridulm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

mridulm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

mridulm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

mridulm Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm commented Nov 8, 2023

abellina commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina commented Nov 16, 2023

tgravescs left a comment

Choose a reason for hiding this comment

mridulm commented Nov 17, 2023

tgravescs commented Nov 17, 2023

abellina commented Nov 17, 2023

dongjoon-hyun left a comment

Choose a reason for hiding this comment

mridulm commented Feb 7, 2024 • edited Loading

dongjoon-hyun commented Feb 7, 2024

mridulm commented Feb 10, 2024

abellina commented Nov 1, 2023 •

edited

Loading

abellina commented Nov 1, 2023 •

edited

Loading

mridulm commented Nov 6, 2023 •

edited

Loading

mridulm commented Nov 7, 2023 •

edited

Loading

mridulm left a comment •

edited

Loading

mridulm Nov 8, 2023 •

edited

Loading

mridulm Nov 8, 2023 •

edited

Loading

mridulm Nov 8, 2023 •

edited

Loading

mridulm Nov 8, 2023 •

edited

Loading

mridulm Nov 8, 2023 •

edited

Loading

mridulm commented Feb 7, 2024 •

edited

Loading