Skip to content

Conversation

@SaintBacchus
Copy link
Contributor

What changes were proposed in this pull request?

In a heavy pressure of the spark application, since the executor will register it to driver block manager twice(because of heart beats), the executor will show as picture show:
image

How was this patch tested?

NA

Details in: SPARK-16868

@SparkQA
Copy link

SparkQA commented Aug 8, 2016

Test build #63335 has finished for PR 14530 at commit 85b385f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public class ShuffleIndexInformation
    • public class ShuffleIndexRecord

}

private def removeDeadExecutorStorageStatus(executorId: String): Unit = {
deadExecutorStorageStatus.zipWithIndex.foreach { case (status, index) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just commenting on the code, not the logic: isn't this just something like deadExecutorStorageStatus.retain(_.blockManagerId.executorId != executorId)? I know it does something slightly different, but maybe it's desirable to clear all such matching statuses. And it's a one-liner that doesn't need a method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retain seems to be a method in MapLike, but I can't find any similar method in ListBuffer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. OK what about

val toRemove = deadExecutorStorageStatus.find(_.blockManagerId.executorId == executorId)
if (toRemove >= 0) {
  deadExecutorStorageStatus.remove(toRemove)
}

Might be clearer and more direct. No big deal either way.

@SparkQA
Copy link

SparkQA commented Aug 8, 2016

Test build #63355 has finished for PR 14530 at commit c065661.

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

@SaintBacchus
Copy link
Contributor Author

\cc @srowen

@srowen
Copy link
Member

srowen commented Aug 11, 2016

As to the actual logic of the change, I don't feel that confident about reviewing it. Suggest looking at 'blame' output here to see who might have written it?

@vanzin
Copy link
Contributor

vanzin commented Aug 11, 2016

Looks fine (small style nit but not worth another review round). I'm a little curious about why it happens - I thought that the driver would kill executors that failed a heartbeat, but that's a separate discussion.

Merging to master.

@asfgit asfgit closed this in 4ec5c36 Aug 11, 2016
@SaintBacchus
Copy link
Contributor Author

I will re-run this case, and dig into why the executor will double register.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants