JoshRosen commented on code in PR #49350: URL: https://github.com/apache/spark/pull/49350#discussion_r1908112009
########## core/src/main/scala/org/apache/spark/storage/BlockManager.scala: ########## @@ -244,6 +245,9 @@ private[spark] class BlockManager( private lazy val maxOnHeapMemory = memoryManager.maxOnHeapStorageMemory private lazy val maxOffHeapMemory = memoryManager.maxOffHeapStorageMemory + // Whether we are launched in a local Spark cluster. + private[spark] val isLocalSparkCluster = Utils.isTesting && + sys.env.contains(ExternalShuffleService.TESTING_ESS_PORT_ENV) Review Comment: Minor nit, but I think this isn't actually directly checking whether we're in local-cluster mode, but, rather, is testing if the ESS port is enabled and is _also_ testing whether Utils.testing is enabled. It also seems a bit weird to me to have this coupling between the BlockManager and ExternalShuffleService class just for the purposes of this check. I think that we can slightly simplify and clean this up by avoiding any changes in `BlockManager` and instead slightly changing the logic inside of ShuffleBlockFetcherIterator. I'll leave a second review comment over there with my suggested approach. -- 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]
