-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-34949][CORE] Prevent BlockManager reregister when Executor is shutting down #32043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,29 +270,36 @@ class ExecutorSuite extends SparkFunSuite | |
| heartbeatZeroAccumulatorUpdateTest(false) | ||
| } | ||
|
|
||
| private def withMockHeartbeatReceiverRef(executor: Executor) | ||
| (func: RpcEndpointRef => Unit): Unit = { | ||
| val executorClass = classOf[Executor] | ||
| val mockReceiverRef = mock[RpcEndpointRef] | ||
| val receiverRef = executorClass.getDeclaredField("heartbeatReceiverRef") | ||
| receiverRef.setAccessible(true) | ||
| receiverRef.set(executor, mockReceiverRef) | ||
|
|
||
| func(mockReceiverRef) | ||
| } | ||
|
|
||
| private def withHeartbeatExecutor(confs: (String, String)*) | ||
| (f: (Executor, ArrayBuffer[Heartbeat]) => Unit): Unit = { | ||
| (f: (Executor, ArrayBuffer[Heartbeat]) => Unit): Unit = { | ||
| val conf = new SparkConf | ||
| confs.foreach { case (k, v) => conf.set(k, v) } | ||
| val serializer = new JavaSerializer(conf) | ||
| val env = createMockEnv(conf, serializer) | ||
| withExecutor("id", "localhost", SparkEnv.get) { executor => | ||
| val executorClass = classOf[Executor] | ||
|
|
||
| // Save all heartbeats sent into an ArrayBuffer for verification | ||
| val heartbeats = ArrayBuffer[Heartbeat]() | ||
| val mockReceiver = mock[RpcEndpointRef] | ||
| when(mockReceiver.askSync(any[Heartbeat], any[RpcTimeout])(any)) | ||
| .thenAnswer((invocation: InvocationOnMock) => { | ||
| val args = invocation.getArguments() | ||
| heartbeats += args(0).asInstanceOf[Heartbeat] | ||
| HeartbeatResponse(false) | ||
| }) | ||
| val receiverRef = executorClass.getDeclaredField("heartbeatReceiverRef") | ||
| receiverRef.setAccessible(true) | ||
| receiverRef.set(executor, mockReceiver) | ||
| withMockHeartbeatReceiverRef(executor) { mockReceiverRef => | ||
| // Save all heartbeats sent into an ArrayBuffer for verification | ||
| val heartbeats = ArrayBuffer[Heartbeat]() | ||
| when(mockReceiverRef.askSync(any[Heartbeat], any[RpcTimeout])(any)) | ||
| .thenAnswer((invocation: InvocationOnMock) => { | ||
| val args = invocation.getArguments() | ||
| heartbeats += args(0).asInstanceOf[Heartbeat] | ||
| HeartbeatResponse(false) | ||
| }) | ||
|
|
||
| f(executor, heartbeats) | ||
| f(executor, heartbeats) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -416,6 +423,35 @@ class ExecutorSuite extends SparkFunSuite | |
| assert(taskMetrics.getMetricValue("JVMHeapMemory") > 0) | ||
| } | ||
|
|
||
| test("SPARK-34949: do not re-register BlockManager when executor is shutting down") { | ||
| val reregisterInvoked = new AtomicBoolean(false) | ||
| val mockBlockManager = mock[BlockManager] | ||
| when(mockBlockManager.reregister()).thenAnswer { (_: InvocationOnMock) => | ||
| reregisterInvoked.getAndSet(true) | ||
| } | ||
| val conf = new SparkConf(false).setAppName("test").setMaster("local[2]") | ||
| val mockEnv = createMockEnv(conf, new JavaSerializer(conf)) | ||
| when(mockEnv.blockManager).thenReturn(mockBlockManager) | ||
|
|
||
| withExecutor("id", "localhost", mockEnv) { executor => | ||
| withMockHeartbeatReceiverRef(executor) { mockReceiverRef => | ||
| when(mockReceiverRef.askSync(any[Heartbeat], any[RpcTimeout])(any)).thenAnswer { | ||
| (_: InvocationOnMock) => HeartbeatResponse(reregisterBlockManager = true) | ||
| } | ||
| val reportHeartbeat = PrivateMethod[Unit](Symbol("reportHeartBeat")) | ||
| executor.invokePrivate(reportHeartbeat()) | ||
| assert(reregisterInvoked.get(), | ||
| "BlockManager.reregister not invoked when reregisterBlockManager was true") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sumeetgajjar . Is this correct,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @dongjoon-hyun , I am slightly confused here about the question. However, if you pointing to the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am assuming @dongjoon-hyun meant to change the message around - "expected foo when bar but was baz".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I meant the message (assertion clue).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the clarification guys, have changed the message and updated the PR 😄. |
||
|
|
||
| reregisterInvoked.getAndSet(false) | ||
| executor.stop() | ||
| executor.invokePrivate(reportHeartbeat()) | ||
| assert(!reregisterInvoked.get(), | ||
| "BlockManager.reregister should not be invoked when executor is stopping") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| test("SPARK-33587: isFatalError") { | ||
| def errorInThreadPool(e: => Throwable): Throwable = { | ||
| intercept[Throwable] { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.