Skip to content
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

[SPARK-17650] malformed url's throw exceptions before bricking Executors #15224

Closed
wants to merge 2 commits into from

Conversation

brkyvz
Copy link
Contributor

@brkyvz brkyvz commented Sep 23, 2016

What changes were proposed in this pull request?

When a malformed URL was sent to Executors through sc.addJar and sc.addFile, the executors become unusable, because they constantly throw MalformedURLExceptions and can never acknowledge that the file or jar is just bad input.

This PR tries to fix that problem by making sure MalformedURLs can never be submitted through sc.addJar and sc.addFile. Another solution would be to blacklist bad files and jars on Executors. Maybe fail the first time, and then ignore the second time (but print a warning message).

How was this patch tested?

Unit tests in SparkContextSuite

@brkyvz
Copy link
Contributor Author

brkyvz commented Sep 23, 2016

cc @rxin

@rxin
Copy link
Contributor

rxin commented Sep 23, 2016

Should we check more in addition to just mailformed urls?

@brkyvz
Copy link
Contributor Author

brkyvz commented Sep 23, 2016

Other places check malformedURIs already (since we always pass around URIs). We can check if the endpoint exists and everything, but that seems a bit heavyweight.

@brkyvz
Copy link
Contributor Author

brkyvz commented Sep 23, 2016

Maybe we also need the blacklisting in case of failures? Maybe they provide a valid URL that doesn't exist, or is unreachable?

@rxin
Copy link
Contributor

rxin commented Sep 23, 2016

cc @zsxwing for review ...

Copy link
Member

@zsxwing zsxwing left a comment

Choose a reason for hiding this comment

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

LGTM. Just one nit.

} catch {
case e: MalformedURLException =>
val msg = s"URI (${uri.toString}) is not a valid URL."
logError(msg)
Copy link
Member

Choose a reason for hiding this comment

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

nit: not need to log it since it's already be thrown.

@brkyvz
Copy link
Contributor Author

brkyvz commented Sep 23, 2016

@zsxwing Thanks for the review. Addressed the nit.

@SparkQA
Copy link

SparkQA commented Sep 23, 2016

Test build #65846 has finished for PR 15224 at commit c65f94f.

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

@SparkQA
Copy link

SparkQA commented Sep 24, 2016

Test build #65854 has finished for PR 15224 at commit 49afc56.

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

@zsxwing
Copy link
Member

zsxwing commented Sep 26, 2016

Thanks. Merging to master and 2.0.

asfgit pushed a commit that referenced this pull request Sep 26, 2016
## What changes were proposed in this pull request?

When a malformed URL was sent to Executors through `sc.addJar` and `sc.addFile`, the executors become unusable, because they constantly throw `MalformedURLException`s and can never acknowledge that the file or jar is just bad input.

This PR tries to fix that problem by making sure MalformedURLs can never be submitted through `sc.addJar` and `sc.addFile`. Another solution would be to blacklist bad files and jars on Executors. Maybe fail the first time, and then ignore the second time (but print a warning message).

## How was this patch tested?

Unit tests in SparkContextSuite

Author: Burak Yavuz <brkyvz@gmail.com>

Closes #15224 from brkyvz/SPARK-17650.

(cherry picked from commit 59d87d2)
Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
@asfgit asfgit closed this in 59d87d2 Sep 26, 2016
@brkyvz brkyvz deleted the SPARK-17650 branch February 3, 2019 20:54
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