Skip to content
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 #1422

Merged
merged 38 commits into from
Jul 2, 2024
Merged

Conversation

jonathanl-bq
Copy link
Collaborator

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.

@jonathanl-bq jonathanl-bq requested a review from a team as a code owner May 17, 2024 16:44
@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label May 17, 2024
@jonathanl-bq
Copy link
Collaborator Author

Panic handling is coming in a separate PR that overhauls how we handle errors in the FFI layer in general.

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jun 29, 2024
5 tasks
@barshaul
Copy link
Collaborator

@jonathanl-bq Ping me when it's ready

@jduo
Copy link
Collaborator

jduo commented Jun 30, 2024

@jonathanl-bq Ping me when it's ready

The panic handling changes got merged already:
#1601

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More logging todos were merged today, sorry.
Please see MessageHandler and #1662

@Yury-Fridlyand
Copy link
Collaborator

@barshaul round

@Yury-Fridlyand Yury-Fridlyand dismissed barshaul’s stale review July 1, 2024 22:32

All PR comments were addressed.

Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Getter private static Level loggerLevel;

private static void initLogger(@NonNull Level level, String fileName) {
if (level == Level.DISABLED) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DISABLED option was added to Java side for testing, since we don't want to log when we have mocks. There's other ways to do it (mocking static objects is annoying, but possible), but this is the best way.

@@ -37,6 +38,7 @@ public class ConnectionWithGlideMockTests extends RustCoreLibMockTestBase {
@BeforeEach
@SneakyThrows
public void createTestClient() {
Logger.setLoggerConfig(Logger.Level.DISABLED);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - this is why we need a DISABLED level logger.
Can we add a TODO to add this level to the redis-core side?

java/src/lib.rs Show resolved Hide resolved
java/src/lib.rs Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand merged commit 5cf478d into valkey-io:main Jul 2, 2024
17 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the java/integ_lotjonat_logging branch July 2, 2024 17:26
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jul 16, 2024
* Implement Logger for Java client

Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Andrew Carbonetto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants