-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
LocalDataSegmentPusher: Fix for Hadoop + relative paths. #1761
Conversation
@@ -54,7 +54,7 @@ public LocalDataSegmentPusher( | |||
@Override | |||
public String getPathForHadoop(String dataSource) | |||
{ | |||
return String.format("file://%s/%s", config.getStorageDirectory(), dataSource); | |||
return String.format("file://%s/%s", config.getStorageDirectory().getAbsoluteFile(), dataSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it at all possible to use standard URI building things here? Either hadoop FS path building or the java File and Path constructs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was avoiding the Hadoop classes since this isn't a hadoop related module. Could probably use the Java stuff a bit more though, let me see
6f9b990
to
fc9f9b7
Compare
fc9f9b7
to
4efbe64
Compare
@drcrallen .toURI.toString works too, updated the patch. It generates different uris ("file:/x/y/z" rather than "file:///x/y/z") but they still work with hadoop. |
"It generates different uris ("file:/x/y/z" rather than "file:///x/y/z") but they still work with hadoop." |
Do you mean, is it a bug that "file:/blah/blah/blah" works? I don't think it's a bug. That's what you get when you do a toURI on a File in Java, so I think it makes sense that it should work in Hadoop. It also appears to also be OK by RFC 3986 in that "file:/blah" is a uri with scheme "file", no authority, and path "/blah". "file:///blah" is the same thing with an empty string authority instead of no authority, which should get interpreted the same way. At least in my reading of the thing.
and
|
LGTM |
LocalDataSegmentPusher: Fix for Hadoop + relative paths.
This makes the path-for-hadoop stuff work with local-mode hadoop when you have a relative local storageDirectory (like simply "segments" if you want to store stuff in segments/ in your current working directory).