Skip to content

Conversation

@srowen
Copy link
Member

@srowen srowen commented Apr 13, 2015

Avoid System.exit(1) in TaskSchedulerImpl and convert to SparkException; ensure scheduler calls sc.stop() even when this exception is thrown.

CC @mateiz @aarondav as those who may have last touched this code.

…on; ensure scheduler calls sc.stop() even when this exception is thrown
@SparkQA
Copy link

SparkQA commented Apr 13, 2015

Test build #30164 has finished for PR 5492 at commit 60dc682.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexer extends Estimator[StringIndexerModel] with StringIndexerBase
    • class VectorAssembler extends Transformer with HasInputCols with HasOutputCol
    • class VectorIndexer extends Estimator[VectorIndexerModel] with VectorIndexerParams
    • case class With(child: LogicalPlan, cteRelations: Map[String, Subquery]) extends UnaryNode
  • This patch does not change any dependencies.

@srowen
Copy link
Member Author

srowen commented Apr 15, 2015

I'd like to commit this. It seems like it solves a problem that's come up a few times now. Continuing with an exception in this corner case instead of forcing immediate termination seems safe; the worst case is that a process limps along a little longer, I believe. It also ensures that a call to sc.stop() happens, as the code seems to intend, but which actually never happened before due to the immediate exit.

@vanzin
Copy link
Contributor

vanzin commented Apr 15, 2015

LGTM.

@sryza
Copy link
Contributor

sryza commented Apr 15, 2015

This seems to only get called from two places:

  • In standalone mode when the driver fails to communicate with the master, or the master sends a message to kill the driver. In these cases, sc.stop is called after.
  • In Mesos mode when Mesos sends a error message to the driver.

Would it maybe make sense to add a call to sc.stop in the Mesos case for consistency?

@srowen
Copy link
Member Author

srowen commented Apr 15, 2015

It might make sense; I just didn't know enough about Mesos to be sure, and so left that alone for now.

@sryza
Copy link
Contributor

sryza commented Apr 15, 2015

Ok. LGTM. Could make sense for someone with Mesos expertise to look at it.

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