-
Notifications
You must be signed in to change notification settings - Fork 0
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
Java: Add Logger #265
Java: Add Logger #265
Conversation
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.
CI is red.
I think we need tests for this feature.
We have few logging TODOs left in client code - can you complete them in scope of this PR? |
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
CI hung. You need to rerun spotless though. |
java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java
Outdated
Show resolved
Hide resolved
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.
looks good - a couple of comments
java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/ReadHandler.java
Outdated
Show resolved
Hide resolved
public final class Logger { | ||
// TODO: consider lazy loading the glide_rs library | ||
static { | ||
System.loadLibrary("glide_rs"); |
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.
can you move the static loading to the ffi.resolvers package?
Example: https://github.com/Bit-Quill/glide-for-redis/blob/c998a2c366b524270a82c1148bfa477f7ab3318c/java/client/src/main/java/glide/ffi/resolvers/SocketListenerResolver.java
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 you can remove this static loader - I don't think it doesn't anything here.
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
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.
Remove the extra static loadlibrary please
public final class Logger { | ||
// TODO: consider lazy loading the glide_rs library | ||
static { | ||
System.loadLibrary("glide_rs"); |
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 you can remove this static loader - I don't think it doesn't anything here.
The Logger implementation here is based on the Python client's implementation, with some relatively small Java/JNI specific changes.