[SPARK-31422][CORE] Fix NPE when BlockManagerSource is used after BlockManagerMaster stops #28187

dongjoon-hyun · 2020-04-11T06:27:03Z

What changes were proposed in this pull request?

This PR (SPARK-31422) aims to return empty result in order to avoid NullPointerException at getStorageStatus and getMemoryStatus which happens after BlockManagerMaster stops. The empty result is consistent with the current status of SparkContext because BlockManager and BlockManagerMaster is already stopped.

Why are the changes needed?

In SparkEnv.stop, the following stop sequence is used and metricsSystem.stop invokes sink.stop.

blockManager.master.stop()
metricsSystem.stop() --> sinks.foreach(_.stop)

However, some sink can invoke BlockManagerSource and ends up with NullPointerException because BlockManagerMaster is already stopped and driverEndpoint became null.

java.lang.NullPointerException
at org.apache.spark.storage.BlockManagerMaster.getStorageStatus(BlockManagerMaster.scala:170)
at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63)
at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63)
at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:31)
at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:30)

Since SparkContext registers and forgets BlockManagerSource without deregistering, we had better avoid NullPointerException inside BlockManagerMaster preventively.

_env.metricsSystem.registerSource(new BlockManagerSource(_env.blockManager))

Does this PR introduce any user-facing change?

Yes. This will remove NPE for the users who uses BlockManagerSource.

How was this patch tested?

Pass the Jenkins with the newly added test cases.

…ckManagerMaster stops

dongjoon-hyun · 2020-04-11T06:38:58Z

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

   * amount of remaining memory.
   */
  def getMemoryStatus: Map[BlockManagerId, (Long, Long)] = {
+    if (driverEndpoint == null) return Map.empty


Although only getStorageStatus is used, I added the preventive code into getMemoryStatus together.

dongjoon-hyun · 2020-04-11T06:54:16Z

Hi, @maropu .
Could you review this PR please?

SparkQA · 2020-04-11T07:05:01Z

Test build #121122 has finished for PR 28187 at commit 438b78b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-04-11T07:06:39Z

retest this please

maropu · 2020-04-11T07:19:50Z

The fix itself is straight-forward and looks fine to me. Btw, (Just a question); couldn't we stop MetricsSystem before stopping BlockManagerMaster even if it depends on BlockManagerMaster?

dongjoon-hyun · 2020-04-11T07:58:58Z

Thank you for reviewing and approval. Yes, it's possible, but the stop sequence has been fixed since Apache Spark 0.8.0-incubating. There might exist a risk because some user sink classes depend on that behavior. So, in this PR, I wanted to focus on this narrow one as a safer solution.

Ngone51 · 2020-04-11T08:05:13Z

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

+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+
+class BlockManagerMasterSuite extends SparkFunSuite {


Add tests to BlockManagerSuite should be enough?

I considered that, but BlockManagerSuite looks misleading to me because this is BlockManagerManger. If there is a more broader suite, it could be a candidate.

I actually think it should be ok because we always test BlockManagerMaster together with BlockManager. And there're already some tests which test BlockManagerMaster only, e.g.

query locations of blockIds

SPARK-30594: Do not post SparkListenerBlockUpdated when updateBlockInfo returns false

Sorry, but I don't think so. In your example, query locations of blockIds is just do mocking instead of testing.

test("query locations of blockIds") { val mockBlockManagerMaster = mock(classOf[BlockManagerMaster])

Also, BlockManagerSuite is already large with 1805 lines. In general, we had better have a proper UT case.

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

dongjoon-hyun · 2020-04-11T08:27:38Z

BTW, for reviewers, during looking at this, I investigated the other sources. The other sources look okay because they don't have risky remote invocation like BlockManagerSource.

DAGSchedulerSource
JVMCPUSource
ExecutorMetricsSource
ExecutorAllocationManagerSource
AppStatusSource
ExternalShuffleServiceSource
MasterSource
WorkerSource
ExecutorSource
PluginMetricsSource
CodegenMetrics
HiveCatalogMetrics
LongAccumulatorSource
DoubleAccumulatorSource
LiveListenerBusMetrics
ApplicationSource
MetricsReporter
StreamingSource

Ngone51 · 2020-04-11T08:29:06Z

LGTM

dongjoon-hyun · 2020-04-11T08:31:50Z

Thank you for review, @Ngone51 .

SparkQA · 2020-04-11T09:35:49Z

Test build #121123 has finished for PR 28187 at commit 438b78b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-11T10:41:57Z

Test build #121126 has finished for PR 28187 at commit 82e46a8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-11T15:27:11Z

Thank you. Merged to master/3.0/2.4.

…ckManagerMaster stops ### What changes were proposed in this pull request? This PR (SPARK-31422) aims to return empty result in order to avoid `NullPointerException` at `getStorageStatus` and `getMemoryStatus` which happens after `BlockManagerMaster` stops. The empty result is consistent with the current status of `SparkContext` because `BlockManager` and `BlockManagerMaster` is already stopped. ### Why are the changes needed? In `SparkEnv.stop`, the following stop sequence is used and `metricsSystem.stop` invokes `sink.stop`. ``` blockManager.master.stop() metricsSystem.stop() --> sinks.foreach(_.stop) ``` However, some sink can invoke `BlockManagerSource` and ends up with `NullPointerException` because `BlockManagerMaster` is already stopped and `driverEndpoint` became `null`. ``` java.lang.NullPointerException at org.apache.spark.storage.BlockManagerMaster.getStorageStatus(BlockManagerMaster.scala:170) at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63) at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63) at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:31) at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:30) ``` Since `SparkContext` registers and forgets `BlockManagerSource` without deregistering, we had better avoid `NullPointerException` inside `BlockManagerMaster` preventively. ```scala _env.metricsSystem.registerSource(new BlockManagerSource(_env.blockManager)) ``` ### Does this PR introduce any user-facing change? Yes. This will remove NPE for the users who uses `BlockManagerSource`. ### How was this patch tested? Pass the Jenkins with the newly added test cases. Closes #28187 from dongjoon-hyun/SPARK-31422. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit a6e6fbf) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ckManagerMaster stops ### What changes were proposed in this pull request? This PR (SPARK-31422) aims to return empty result in order to avoid `NullPointerException` at `getStorageStatus` and `getMemoryStatus` which happens after `BlockManagerMaster` stops. The empty result is consistent with the current status of `SparkContext` because `BlockManager` and `BlockManagerMaster` is already stopped. ### Why are the changes needed? In `SparkEnv.stop`, the following stop sequence is used and `metricsSystem.stop` invokes `sink.stop`. ``` blockManager.master.stop() metricsSystem.stop() --> sinks.foreach(_.stop) ``` However, some sink can invoke `BlockManagerSource` and ends up with `NullPointerException` because `BlockManagerMaster` is already stopped and `driverEndpoint` became `null`. ``` java.lang.NullPointerException at org.apache.spark.storage.BlockManagerMaster.getStorageStatus(BlockManagerMaster.scala:170) at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63) at org.apache.spark.storage.BlockManagerSource$$anonfun$10.apply(BlockManagerSource.scala:63) at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:31) at org.apache.spark.storage.BlockManagerSource$$anon$1.getValue(BlockManagerSource.scala:30) ``` Since `SparkContext` registers and forgets `BlockManagerSource` without deregistering, we had better avoid `NullPointerException` inside `BlockManagerMaster` preventively. ```scala _env.metricsSystem.registerSource(new BlockManagerSource(_env.blockManager)) ``` ### Does this PR introduce any user-facing change? Yes. This will remove NPE for the users who uses `BlockManagerSource`. ### How was this patch tested? Pass the Jenkins with the newly added test cases. Closes apache#28187 from dongjoon-hyun/SPARK-31422. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-31422][CORE] Fix NPE when BlockManagerSource is used after Blo…

438b78b

…ckManagerMaster stops

dongjoon-hyun added the SPARK CORE label Apr 11, 2020

dongjoon-hyun commented Apr 11, 2020

View reviewed changes

maropu approved these changes Apr 11, 2020

View reviewed changes

Ngone51 reviewed Apr 11, 2020

View reviewed changes

Add JIRA id

82e46a8

dongjoon-hyun closed this in a6e6fbf Apr 11, 2020

dongjoon-hyun deleted the SPARK-31422 branch April 11, 2020 15:29


		import org.apache.spark.{SparkConf, SparkFunSuite}

		class BlockManagerMasterSuite extends SparkFunSuite {

[SPARK-31422][CORE] Fix NPE when BlockManagerSource is used after BlockManagerMaster stops #28187

[SPARK-31422][CORE] Fix NPE when BlockManagerSource is used after BlockManagerMaster stops #28187

Uh oh!

Conversation

dongjoon-hyun commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

dongjoon-hyun Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Apr 11, 2020

Uh oh!

SparkQA commented Apr 11, 2020

Uh oh!

maropu commented Apr 11, 2020

Uh oh!

maropu commented Apr 11, 2020

Uh oh!

dongjoon-hyun commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ngone51 Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51 Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

Ngone51 Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ngone51 commented Apr 11, 2020

Uh oh!

dongjoon-hyun commented Apr 11, 2020

Uh oh!

SparkQA commented Apr 11, 2020

Uh oh!

SparkQA commented Apr 11, 2020

Uh oh!

dongjoon-hyun commented Apr 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongjoon-hyun commented Apr 11, 2020 •

edited

Loading

dongjoon-hyun Apr 11, 2020 •

edited

Loading

dongjoon-hyun commented Apr 11, 2020 •

edited

Loading

dongjoon-hyun Apr 11, 2020 •

edited

Loading

dongjoon-hyun commented Apr 11, 2020 •

edited

Loading