-
Notifications
You must be signed in to change notification settings - Fork 45
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
RasterRef should not read HDFS scheme with GDAL reader. #319
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Jason T. Brown <[email protected]>
Signed-off-by: Jason T. Brown <[email protected]>
core/src/main/scala/org/locationtech/rasterframes/ref/RasterSource.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/locationtech/rasterframes/ref/RasterSourceSpec.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Jason T. Brown <[email protected]>
Still having some class not found errors in travis. Unsure why. |
@vpipkt I think something's wrong with the implementation, because in this test case the scheme resolution should not be attempting to use the GDAL reader if it's not available, yet it's still selecting it over the default reader. https://travis-ci.org/locationtech/rasterframes/builds/580393491#L832-L866 |
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 think there's a bug causing GDAL reader to be selected when GDAL not available and JVM one sufficient.
@vpipkt Not 100% sure what's going on here, but don't want to be extra sure the JVM fallback reader still gets selected in non-GDAL scenarios. Wonder if we should leverage this sort of logic? rasterframes/.circleci/config.yml Line 126 in 36be700
Also odd: not seeing it here: |
Now you can see in Circle CI, it is passing. https://circleci.com/gh/s22s/rasterframes/tree/fix%2F314 |
Signed-off-by: Jason T. Brown <[email protected]>
@@ -78,7 +78,7 @@ object GDALRasterSource extends LazyLogging { | |||
val _ = new GDALWarp() | |||
true | |||
} catch { | |||
case _: UnsatisfiedLinkError => | |||
case _:UnsatisfiedLinkError | _:NoClassDefFoundError => |
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.
@metasim I suspect this may be a terrible idea.
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.
Hmmmmmm. Interesting......
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 don't understand why Travis isn't passing yet. CircleCI doesn't surprise me because GDAL is installed. I've not confirmed that it is in the Travis configuration.
@vpipkt Once this weirdness is resolved, should we be trying to use this (if memory serves, written by Azavea folk!)?: https://gdal.org/user/virtual_file_systems.html#vsihdfs-hadoop-file-system |
@metasim yep, by @jamesmcclain |
I am here to help if you run into any trouble with that. Please be advised that some users have run into issues when trying to access HDFS filesystems through GDAL from a JVM process. The problem appears to be related to the fact that GDAL spawns its own inferior JVM to interface with HDFS, and that is done from within a JVM process running native GDAL code (more here). EDIT: Specifically, please see this comment and surrounding comments. There does not appear to be a problem on OpenJDK 11, but there is a problem with OpenJDK 8. |
@jamesmcclain Thanks for the background color... sounds to me like we shouldn't go that route if we don't have to. |
You are welcome. |
844c524
to
a5c6446
Compare
And enforce other logic like can't read a gdal only format (mrf, jp2) outside the gdal reader.
Needs a close look and maybe more extensive unit tests. Possibly also more extensive / detailed error messages.
For #314
This also relates, from the user perspective, to #267