Java: Add Logger #1422
Java: Add Logger #1422
Conversation
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Java: Add Logger
Yury-Fridlyand
requested review from
aaron-congo,
barshaul,
Yury-Fridlyand,
acarbonetto and
shohamazon
There was a problem hiding this comment.
IMPORTANT
I see that you use panic and unwrap throughout the lib.rs file, which might crash the user's application, which is definitely something that our client shouldn't do. Have you tested how these unwrap/panics points behave?
In node and python we use Results to return errors, both on the logger and in the value_from_pointer function.
Please check what is the correct way in JNI to handle exceptions, or if it has some JResult object, and test to see how an application would behave when an exception is thrown in the rust code.
See jni-rs/jni-rs#485, might be helpful
| @Getter private static Level loggerLevel; | ||
|
|
||
| private static void initLogger(@NonNull Level level, String fileName) { | ||
| if (level == Level.DISABLED) { |
There was a problem hiding this comment.
Since we dont have DISABLED option in node and python - can you please open another PR that adds this option to the other wrappers too?
| @@ -37,6 +38,7 @@ public class ConnectionWithGlideMockTests extends RustCoreLibMockTestBase { | |||
| @BeforeEach | |||
| @SneakyThrows | |||
| public void createTestClient() { | |||
| Logger.setLoggerConfig(Logger.Level.DISABLED); | |||
There was a problem hiding this comment.
We needed to disable logging to get these tests to work. They're only unit tests and shouldn't be using a real logger anyway.
| @@ -134,7 +134,6 @@ tasks.withType(Test) { | |||
| events "started", "skipped", "passed", "failed" | |||
| showStandardStreams true | |||
| } | |||
| jvmArgs "-Djava.library.path=${project.rootDir}/target/release" | |||
There was a problem hiding this comment.
This is no longer needed now that we use the NativeLibLoader. It would've been better to put this in the PR that had the changes to publish to local Maven repository, but that's already been merged.
I haven't witnessed those |
|
Yes please add test to check that we handle properly panics in rust |
|
Panic handling is coming in a separate PR that overhauls how we handle errors in the FFI layer in general. |
Issue #, if available:
N/A
The Logger implementation here is based on the Python client's implementation, with some relatively small Java/JNI specific changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.