chore: Improve error handling when native lib fails to load #1000
Conversation
andygrove
changed the title
|
@parthchandra could you review? |
| @@ -1105,7 +1105,8 @@ object CometSparkSessionExtensions extends Logging { | |||
| try { | |||
| // This will load the Comet native lib on demand, and if success, should set | |||
| // `NativeBase.loaded` to true | |||
| NativeBase.isLoaded | |||
| NativeBase.load() | |||
There was a problem hiding this comment.
I'm surprised we have to do this. I always understood that the static initializer would be called before we can invoke a static method.
There was a problem hiding this comment.
The goal of this PR was to remove the static initializer and explicitly load the native library so that we can catch exceptions and report them.
| @@ -24,6 +24,13 @@ | |||
| import org.apache.comet.NativeBase; | |||
|
|
|||
| public final class Native extends NativeBase { | |||
|
|
|||
| static { | |||
There was a problem hiding this comment.
Any reason for moving this from the base class?
There was a problem hiding this comment.
It turns out that we still rely on the static initializer for the native Parquet code, which I had not realized when I started on this PR.
There was a problem hiding this comment.
Hmm, if only scan is enabled, can we still catch the exception whiling unable to load the native lib?
There was a problem hiding this comment.
We could potentially catch UnsatisfiedLinkError (and other exceptions) in NativeBase.bundleLoadLibrary or NativeBase.load. But this being static initialization, I'm not sure if the logger would have been initialized at that point.
andygrove
force-pushed
the
remove-static-init
branch
from
0fc833b to
9c54977
Compare
|
@parthchandra @viirya I reimplemented this with a much simpler approach. PTAL when you can. |
| private static final String searchPattern = "libcomet-"; | ||
|
|
||
| static { | ||
| if (!isLoaded()) { |
There was a problem hiding this comment.
load is a no-op if loaded == false so no need for this conditional here
| } | ||
| } | ||
|
|
||
| public static synchronized boolean isLoaded() { | ||
| public static synchronized boolean isLoaded() throws Throwable { |
There was a problem hiding this comment.
This is called from isCometEnabled and we already had error handling here which could never be triggered due to isLoaded never being capable of throwing an exception.
There was a problem hiding this comment.
Correction: calling isLoaded could have resulted in the NoClassDefFoundError but this did not have details of root cause.
andygrove
changed the title
Which issue does this PR close?
Closes #999
Rationale for this change
If the static initialization block failed to load the library then we did not see the reason why (in some cases).
Before:
After:
What changes are included in this PR?
Improve error handling by consuming exceptions in the static init code and then rethrowing them when
isCometEnabledcallsisLibraryLoadedHow are these changes tested?
Manually.