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

TOREE-408: Add support for hdfs and s3 to AddJar. #125

Conversation

rdblue
Copy link
Contributor

@rdblue rdblue commented Jun 10, 2017

No description provided.

@LK-Tmac1
Copy link

How to know the reason for the failure of the test? The details said 1 Test Failed for org.apache.toree.magic.builtin.AddJarSpec but no further explanations.

@jodersky
Copy link
Member

jodersky commented Jun 12, 2017

@liukun1016 It's a bit further up, line 3324.

rdblue added 2 commits June 18, 2017 14:32
URI removes the query fragment from the path component and file name is
based on the path. This test used to rely on the fact that the query
fragment was included in the file name returned by URL, but this is
unreliable. The URL could easily end with some other query param instead
of the file name, so the logic wasn't reliable.
@rdblue rdblue force-pushed the TOREE-408-add-jar-support-hadoop-file-systems branch from b1b436f to 4b2134d Compare June 18, 2017 21:44
@rdblue
Copy link
Contributor Author

rdblue commented Jun 18, 2017

Rebased on master and fixed the test failure.

The problem was with the test itself. The addJar code was returning the file name based on a query param instead of the URL's path. Since query params can be in any order and can't be relied upon for correct file names, the right thing to do is to get the filename from the path component. I updated the test to do that.

@rdblue
Copy link
Contributor Author

rdblue commented Jun 19, 2017

@kamir, I think this fixes #118 because URI correctly parses the file name without query params. Can you have a look?

@lresende
Copy link
Member

LGTM, merging if there are no more comments.

@asfgit asfgit closed this in 79d02e2 Jul 20, 2017
lresende pushed a commit to lresende/incubator-toree that referenced this pull request Oct 5, 2017
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.

5 participants