Skip to content

Conversation

@Parth-Brahmbhatt
Copy link
Contributor

@Parth-Brahmbhatt Parth-Brahmbhatt commented Aug 19, 2016

What changes were proposed in this pull request?

Add jar command fails when given s3n or hdfs urls. This changes fixes that.

How was this patch tested?

unit test added, it was just copied from a previously opened PR #10797 so credit should be given to the person who originally posted the patch.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@Parth-Brahmbhatt
Copy link
Contributor Author

Can one of the committers take a look at this PR?

1 similar comment
@Parth-Brahmbhatt
Copy link
Contributor Author

Can one of the committers take a look at this PR?

@Parth-Brahmbhatt
Copy link
Contributor Author

Request for review one more time.

@Parth-Brahmbhatt
Copy link
Contributor Author

Ping.

@Parth-Brahmbhatt
Copy link
Contributor Author

Request for review.

@felixcheung
Copy link
Member

@srowen would this make sense to you?

val testJar = "hdfs://nn:8020/foo.jar"
// This should fail with unknown host, as its just testing the URL parsing
// before SPARK-12868 it was failing with Malformed URI
val e = intercept[RuntimeException] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test should be improved before merging this. Looking for a RuntimeException to validate that the Jar was registered is brittle and can easily pass when the registration doesn't actually work.

@HyukjinKwon
Copy link
Member

Hi @Parth-Brahmbhatt, is it still active? If so is there any argument against #14720 (comment)?

@wangyum
Copy link
Member

wangyum commented May 11, 2017

It has been fixed by #17342.

@HyukjinKwon
Copy link
Member

I will propose to close this after a week if it is still inactive or no objection to ^.

@Parth-Brahmbhatt
Copy link
Contributor Author

closing.

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.

7 participants