Skip to content

Conversation

@tedyu
Copy link
Contributor

@tedyu tedyu commented Oct 23, 2015

In two places in Outbox.scala , new SparkException is created for Outbox stopped condition.
Create a singleton for Outbox stopped exception and use it instead of creating exception every time.

@tedyu tedyu changed the title Make Outbox stopped exception singleton SPARK-11286 Make Outbox stopped exception singleton Oct 23, 2015
@rxin
Copy link
Contributor

rxin commented Oct 23, 2015

Ted - why is this change necessary? These only happen in error cases so I'm not sure how useful it is to optimize then. On top of that, this seems to change the stacktrace coming from the exceptions.

@vanzin
Copy link
Contributor

vanzin commented Oct 23, 2015

Yeah, please don't do this. Broken stack traces are terrible.

@tedyu tedyu closed this Oct 23, 2015
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exception creation can be done here instead of inside the loop, keeping usable stack trace

Copy link
Contributor

Choose a reason for hiding this comment

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

But why? What's being optimized here? It's such a rare occurrence that it's better to pay the allocation cost when it happens than waste the memory to keep the singleton around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case the exception is no longer a singleton - it is created inside handleNetworkFailure().
Network failure should be rare but when it happens, there may be non-trivial number of messages to be notified.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt there will be many messages in the outbox at a time. This code mostly fixes a race during initialization of the RPC channel, later the outbox would be mostly a bypass. #9210 even bypasses the outbox altogether in a lot of cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for providing richer background information.

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44230 has finished for PR 9254 at commit 79c6e00.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class Data(feature: Int)\n

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