From fec36b16b3cdd153ff9d013cb71c7e337071ee38 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 6 May 2024 17:26:27 -0700 Subject: [PATCH 01/33] Implement Logger for Java client --- .../main/java/glide/api/logging/Logger.java | 84 +++++++++++++++++++ java/client/src/main/java/module-info.java | 1 + java/src/lib.rs | 78 ++++++++++++++++- 3 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 java/client/src/main/java/glide/api/logging/Logger.java diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java new file mode 100644 index 0000000000..a7994983b0 --- /dev/null +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -0,0 +1,84 @@ +package glide.api.logging; + +import lombok.Getter; +import lombok.NonNull; + +public class Logger { + // TODO: consider lazy loading the glide_rs library + static { + System.loadLibrary("glide_rs"); + } + + // Enum ordinal is used, so order of variants must be kept the same + @Getter + public enum Level { + DISABLED(-1), + ERROR(0), + WARN(1), + INFO(2), + DEBUG(3), + TRACE(4); + + private final int level; + + private Level(int level) { + this.level = level; + } + } + + private static Logger instance; + private static Level loggerLevel; + + private Logger(@NonNull Level level, String fileName) { + loggerLevel = Level.values()[initInternal(level.getLevel(), fileName)]; + } + + private Logger(String fileName) { + this(Level.DISABLED, fileName); + } + + private Logger(@NonNull Level level) { + this(level, null); + } + + private Logger() { + this(Level.DISABLED, null); + } + + public static void init(Level level, String fileName) { + if (instance == null) { + instance = new Logger(level, fileName); + } + } + + public static void init(String fileName) { + init(null, fileName); + } + + public static void init() { + init(null, null); + } + + public static void init(Level level) { + init(level, null); + } + + public static void log(@NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { + if (instance == null) { + instance = new Logger(Level.DISABLED, null); + } + if (!(level.getLevel() <= loggerLevel.getLevel())) { + return; + } + logInternal(level.getLevel(), logIdentifier, message); + } + + public void setLoggerConfig(Level level, String fileName) { + instance = new Logger(level, fileName); + } + + private static native int initInternal(int level, String fileName); + + private static native void logInternal(int level, String logIdentifier, String message); + +} diff --git a/java/client/src/main/java/module-info.java b/java/client/src/main/java/module-info.java index 4500dc538e..5a2085fef5 100644 --- a/java/client/src/main/java/module-info.java +++ b/java/client/src/main/java/module-info.java @@ -1,6 +1,7 @@ module glide.api { exports glide.api; exports glide.api.commands; + exports glide.api.logging; exports glide.api.models; exports glide.api.models.commands; exports glide.api.models.commands.stream; diff --git a/java/src/lib.rs b/java/src/lib.rs index eb81b165f1..5609be0b35 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -3,8 +3,9 @@ */ use glide_core::start_socket_listener; +use jni::errors::Error as JniError; use jni::objects::{JClass, JObject, JObjectArray, JString, JThrowable}; -use jni::sys::jlong; +use jni::sys::{jint, jlong}; use jni::JNIEnv; use log::error; use redis::Value; @@ -15,6 +16,8 @@ mod ffi_test; #[cfg(ffi_test)] pub use ffi_test::*; +struct Level(i32); + // TODO: Consider caching method IDs here in a static variable (might need RwLock to mutate) fn redis_value_to_java<'local>(env: &mut JNIEnv<'local>, val: Value) -> JObject<'local> { match val { @@ -176,3 +179,76 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local let hash_str: String = env.get_string(&hash).unwrap().into(); glide_core::scripts_container::remove_script(&hash_str); } + +impl From for Level { + fn from(level: logger_core::Level) -> Self { + match level { + logger_core::Level::Error => Level(0), + logger_core::Level::Warn => Level(1), + logger_core::Level::Info => Level(2), + logger_core::Level::Debug => Level(3), + logger_core::Level::Trace => Level(4), + } + } +} + +impl TryFrom for logger_core::Level { + type Error = String; + fn try_from(level: Level) -> Result>::Error> { + match level.0 { + 0 => Ok(logger_core::Level::Error), + 1 => Ok(logger_core::Level::Warn), + 2 => Ok(logger_core::Level::Info), + 3 => Ok(logger_core::Level::Debug), + 4 => Ok(logger_core::Level::Trace), + _ => Err(format!("Invalid log level: {:?}", level.0)), + } + } +} + +#[no_mangle] +pub extern "system" fn Java_glide_utils_Logger_logInternal<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + level: jint, + log_identifier: JString<'local>, + message: JString<'local>, +) { + let level = Level(level); + + let log_identifier: String = env + .get_string(&log_identifier) + .expect(make_jstring_error("log_identifier").as_str()) + .into(); + + let message: String = env + .get_string(&message) + .expect(make_jstring_error("message").as_str()) + .into(); + + logger_core::log(level.try_into().unwrap(), log_identifier, message); +} + +#[no_mangle] +pub extern "system" fn Java_glide_utils_Logger_initInternal<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + level: jint, + file_name: JString<'local>, +) -> jint { + let level = if level > 0 { Some(level) } else { None }; + let file_name: Option = match env.get_string(&file_name) { + Ok(file_name) => Some(file_name.into()), + Err(JniError::NullPtr(_)) => None, + _ => panic!(make_jstring_error("file_name")), + }; + let logger_level = logger_core::init( + level.map(|level| Level(level).try_into().unwrap()), + file_name.as_deref(), + ); + Level::from(logger_level).0 +} + +fn make_jstring_error(jstring: &str) -> String { + format!("Failed to get String from JString {}", jstring) +} From 24af480daec133396e7f7eddf091c141b928e2bb Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 6 May 2024 17:28:09 -0700 Subject: [PATCH 02/33] Run spotless --- java/client/src/main/java/glide/api/logging/Logger.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index a7994983b0..2da0d43b82 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -1,3 +1,4 @@ +/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.api.logging; import lombok.Getter; @@ -63,7 +64,8 @@ public static void init(Level level) { init(level, null); } - public static void log(@NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { + public static void log( + @NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { if (instance == null) { instance = new Logger(Level.DISABLED, null); } @@ -80,5 +82,4 @@ public void setLoggerConfig(Level level, String fileName) { private static native int initInternal(int level, String fileName); private static native void logInternal(int level, String logIdentifier, String message); - } From d3b2f813101a6b897f5cff7e6bc78cd1f24c6473 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 7 May 2024 10:53:15 -0700 Subject: [PATCH 03/33] Annotate NonNull for Level in public API --- .../client/src/main/java/glide/api/logging/Logger.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 2da0d43b82..4fdbe02dfc 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -46,21 +46,21 @@ private Logger() { this(Level.DISABLED, null); } - public static void init(Level level, String fileName) { + public static void init(@NonNull Level level, String fileName) { if (instance == null) { instance = new Logger(level, fileName); } } public static void init(String fileName) { - init(null, fileName); + init(Level.DISABLED, fileName); } public static void init() { - init(null, null); + init(Level.DISABLED, null); } - public static void init(Level level) { + public static void init(@NonNull Level level) { init(level, null); } @@ -75,7 +75,7 @@ public static void log( logInternal(level.getLevel(), logIdentifier, message); } - public void setLoggerConfig(Level level, String fileName) { + public void setLoggerConfig(@NonNull Level level, String fileName) { instance = new Logger(level, fileName); } From e21f7732a5871227a0c1273c6f91fc546e61b2bc Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 7 May 2024 10:58:21 -0700 Subject: [PATCH 04/33] Fix build error --- java/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 5609be0b35..0c00edb95e 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -240,7 +240,7 @@ pub extern "system" fn Java_glide_utils_Logger_initInternal<'local>( let file_name: Option = match env.get_string(&file_name) { Ok(file_name) => Some(file_name.into()), Err(JniError::NullPtr(_)) => None, - _ => panic!(make_jstring_error("file_name")), + _ => panic!("{}", make_jstring_error("file_name")), }; let logger_level = logger_core::init( level.map(|level| Level(level).try_into().unwrap()), From 604066ed2ff6bdcef58c8568b81b4a89f7e4b5bc Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 7 May 2024 11:20:06 -0700 Subject: [PATCH 05/33] Fix clippy lint --- java/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 0c00edb95e..dbf16206a7 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -218,12 +218,12 @@ pub extern "system" fn Java_glide_utils_Logger_logInternal<'local>( let log_identifier: String = env .get_string(&log_identifier) - .expect(make_jstring_error("log_identifier").as_str()) + .unwrap_or_else(|_| panic!("{}", make_jstring_error("log_identifier"))) .into(); let message: String = env .get_string(&message) - .expect(make_jstring_error("message").as_str()) + .unwrap_or_else(|_| panic!("{}", make_jstring_error("message"))) .into(); logger_core::log(level.try_into().unwrap(), log_identifier, message); From 18509b3aeb6e1dca6052ce57ea9f10b8f182202d Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 13 May 2024 10:33:53 -0700 Subject: [PATCH 06/33] Update logger --- .../main/java/glide/api/logging/Logger.java | 121 +++++++++++++++--- .../handlers/CallbackDispatcher.java | 6 +- .../connectors/handlers/ReadHandler.java | 4 +- .../ConnectionWithGlideMockTests.java | 4 + .../src/test/java/glide/LoggerTests.java | 29 +++++ 5 files changed, 140 insertions(+), 24 deletions(-) create mode 100644 java/integTest/src/test/java/glide/LoggerTests.java diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 4fdbe02dfc..caf3a6311d 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -4,7 +4,16 @@ import lombok.Getter; import lombok.NonNull; -public class Logger { +/** + * A singleton class that allows logging which is consistent with logs from the internal rust core. + * The logger can be set up in 2 ways -
+ * 1. By calling Logger.init, which configures the logger only if it wasn't previously configured.
+ * 2. By calling Logger.setLoggerConfig, which replaces the existing configuration, and means that new logs will not be + * saved with the logs that were sent before the call.

+ * If setLoggerConfig wasn't called, the first log attempt will initialize a new logger with default configuration decided + * by the Rust core. + */ +public final class Logger { // TODO: consider lazy loading the glide_rs library static { System.loadLibrary("glide_rs"); @@ -13,7 +22,7 @@ public class Logger { // Enum ordinal is used, so order of variants must be kept the same @Getter public enum Level { - DISABLED(-1), + DEFAULT(-1), ERROR(0), WARN(1), INFO(2), @@ -27,47 +36,84 @@ private Level(int level) { } } - private static Logger instance; + @Getter private static Level loggerLevel; - private Logger(@NonNull Level level, String fileName) { + private static void initLogger(@NonNull Level level, String fileName) { loggerLevel = Level.values()[initInternal(level.getLevel(), fileName)]; } - private Logger(String fileName) { - this(Level.DISABLED, fileName); + private static void initLogger(String fileName) { + initLogger(Level.DEFAULT, fileName); } - private Logger(@NonNull Level level) { - this(level, null); + private static void initLogger(@NonNull Level level) { + initLogger(level, null); } - private Logger() { - this(Level.DISABLED, null); + private static void initLogger() { + initLogger(Level.DEFAULT, null); } + /** + * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to + * replace an existing logger. + * The logger will filter all logs with a level lower than the given level. + * If given a fileName argument, will write the logs to files postfixed with fileName. If fileName isn't provided, + * the logs will be written to the console. + * + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. + * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + * @param fileName If provided, the target of the logs will be the file mentioned. + * Otherwise, logs will be printed to the console. + */ public static void init(@NonNull Level level, String fileName) { - if (instance == null) { - instance = new Logger(level, fileName); + if (loggerLevel == null) { + initLogger(level, fileName); } } + /** + * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to + * replace an existing logger. + * The logger will filter all logs with a level lower than the default level decided by the Rust core. + * If given a fileName argument, will write the logs to files postfixed with fileName. If fileName isn't provided, + * the logs will be written to the console. + * + * @param fileName If provided, the target of the logs will be the file mentioned. + * Otherwise, logs will be printed to the console. + */ public static void init(String fileName) { - init(Level.DISABLED, fileName); + init(Level.DEFAULT, fileName); } + /** + * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to + * replace an existing logger. + * The logger will filter all logs with a level lower than the default level decided by the Rust core. + * The logs will be written to the console. + */ public static void init() { - init(Level.DISABLED, null); + init(Level.DEFAULT, null); } + /** + * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to + * replace an existing logger. + * The logger will filter all logs with a level lower than the given level. + * The logs will be written to the console. + * + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. + * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + */ public static void init(@NonNull Level level) { init(level, null); } public static void log( @NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { - if (instance == null) { - instance = new Logger(Level.DISABLED, null); + if (loggerLevel == null) { + initLogger(Level.DEFAULT, null); } if (!(level.getLevel() <= loggerLevel.getLevel())) { return; @@ -75,8 +121,47 @@ public static void log( logInternal(level.getLevel(), logIdentifier, message); } - public void setLoggerConfig(@NonNull Level level, String fileName) { - instance = new Logger(level, fileName); + /** + * Creates a new logger instance and configure it with the provided log level and file name. + * + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. + * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + * @param fileName If provided, the target of the logs will be the file mentioned. + * Otherwise, logs will be printed to the console. + */ + public static void setLoggerConfig(@NonNull Level level, String fileName) { + initLogger(level, fileName); + } + + /** + * Creates a new logger instance and configure it with the provided log level and file name. + * The logs will be written to the console. + * + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. + * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + */ + public static void setLoggerConfig(@NonNull Level level) { + setLoggerConfig(level, null); + } + + /** + * Creates a new logger instance and configure it with the provided log level and file name. + * The logger will filter all logs with a level lower than the default level decided by the Rust core. + * + * @param fileName If provided, the target of the logs will be the file mentioned. + * Otherwise, logs will be printed to the console. + */ + public static void setLoggerConfig(String fileName) { + setLoggerConfig(Level.DEFAULT, fileName); + } + + /** + * Creates a new logger instance and configure it with the provided log level and file name. + * The logger will filter all logs with a level lower than the default level decided by the Rust core. + * The logs will be written to the console. + */ + public static void setLoggerConfig() { + setLoggerConfig(Level.DEFAULT, null); } private static native int initInternal(int level, String fileName); diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index 6c5e86e2d2..34f4efa0ba 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -1,6 +1,7 @@ /** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.connectors.handlers; +import glide.api.logging.Logger; import glide.api.models.exceptions.ClosingException; import glide.api.models.exceptions.ConnectionException; import glide.api.models.exceptions.ExecAbortException; @@ -107,11 +108,8 @@ public void completeRequest(Response response) { } future.completeAsync(() -> response); } else { - // TODO: log an error thru logger. // probably a response was received after shutdown or `registerRequest` call was missing - System.err.printf( - "Received a response for not registered callback id %d, request error = %s%n", - callbackId, response.getRequestError()); + Logger.log(Logger.Level.ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError() + "%n"); distributeClosingException("Client is in an erroneous state and should close"); } } diff --git a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java index 29b7f4c01b..4431c64be4 100644 --- a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java @@ -1,6 +1,7 @@ /** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.connectors.handlers; +import glide.api.logging.Logger; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; import lombok.NonNull; @@ -29,8 +30,7 @@ public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg) /** Handles uncaught exceptions from {@link #channelRead(ChannelHandlerContext, Object)}. */ @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - // TODO: log thru logger - System.out.printf("=== exceptionCaught %s %s %n", ctx, cause); + Logger.log(Logger.Level.ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause + " %n"); callbackDispatcher.distributeClosingException( "An unhandled error while reading from UDS channel: " + cause); diff --git a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java index 08235ac1fc..8fb3653e3b 100644 --- a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java +++ b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java @@ -7,10 +7,12 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; import connection_request.ConnectionRequestOuterClass.ConnectionRequest; import connection_request.ConnectionRequestOuterClass.NodeAddress; import glide.api.RedisClient; +import glide.api.logging.Logger; import glide.api.models.exceptions.ClosingException; import glide.connectors.handlers.CallbackDispatcher; import glide.connectors.handlers.ChannelHandler; @@ -27,6 +29,8 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; import redis_request.RedisRequestOuterClass.RedisRequest; import response.ResponseOuterClass.Response; diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java new file mode 100644 index 0000000000..d0004f4dee --- /dev/null +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -0,0 +1,29 @@ +package glide; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import glide.api.logging.Logger; +import org.junit.jupiter.api.Test; + +public class LoggerTests { + + private final Logger.Level DEFAULT_TEST_LOG_LEVEL = Logger.Level.WARN; + + @Test + public void init_logger() { + Logger.init(DEFAULT_TEST_LOG_LEVEL); + assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + // The logger is already configured, so calling init again shouldn't modify the log level + Logger.init(Logger.Level.ERROR); + assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + } + + @Test + public void set_logger_config() { + Logger.setLoggerConfig(Logger.Level.INFO); + assertEquals(Logger.getLoggerLevel(), Logger.Level.INFO); + // Revert to the default test log level + Logger.setLoggerConfig(DEFAULT_TEST_LOG_LEVEL); + assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + } +} From a291ad137ff26cba16a696a28672dd520f404e49 Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Mon, 13 May 2024 10:44:23 -0700 Subject: [PATCH 07/33] Fix FFI function names. Signed-off-by: Yury-Fridlyand --- java/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index dbf16206a7..71dd834803 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -207,7 +207,7 @@ impl TryFrom for logger_core::Level { } #[no_mangle] -pub extern "system" fn Java_glide_utils_Logger_logInternal<'local>( +pub extern "system" fn Java_glide_api_logging_Logger_logInternal<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, level: jint, @@ -230,7 +230,7 @@ pub extern "system" fn Java_glide_utils_Logger_logInternal<'local>( } #[no_mangle] -pub extern "system" fn Java_glide_utils_Logger_initInternal<'local>( +pub extern "system" fn Java_glide_api_logging_Logger_initInternal<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, level: jint, From 73fc219bb3621fdd3571c8b16d141f8cb0d84fa3 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 13 May 2024 10:48:10 -0700 Subject: [PATCH 08/33] Remove %n from logs --- .../main/java/glide/connectors/handlers/CallbackDispatcher.java | 2 +- .../src/main/java/glide/connectors/handlers/ReadHandler.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index 34f4efa0ba..30398cc9c7 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -109,7 +109,7 @@ public void completeRequest(Response response) { future.completeAsync(() -> response); } else { // probably a response was received after shutdown or `registerRequest` call was missing - Logger.log(Logger.Level.ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError() + "%n"); + Logger.log(Logger.Level.ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError()); distributeClosingException("Client is in an erroneous state and should close"); } } diff --git a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java index 4431c64be4..389889e034 100644 --- a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java @@ -30,7 +30,7 @@ public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg) /** Handles uncaught exceptions from {@link #channelRead(ChannelHandlerContext, Object)}. */ @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - Logger.log(Logger.Level.ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause + " %n"); + Logger.log(Logger.Level.ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause); callbackDispatcher.distributeClosingException( "An unhandled error while reading from UDS channel: " + cause); From c998a2c366b524270a82c1148bfa477f7ab3318c Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 13 May 2024 12:07:32 -0700 Subject: [PATCH 09/33] Fix Logger test --- .../src/main/java/glide/api/logging/Logger.java | 16 +++++++++++++--- .../src/test/java/glide/LoggerTests.java | 8 ++++---- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index caf3a6311d..fc2b033aa4 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -19,7 +19,6 @@ public final class Logger { System.loadLibrary("glide_rs"); } - // Enum ordinal is used, so order of variants must be kept the same @Getter public enum Level { DEFAULT(-1), @@ -31,16 +30,27 @@ public enum Level { private final int level; - private Level(int level) { + Level(int level) { this.level = level; } + + public static Level fromInt(int i) { + switch (i) { + case 0: return ERROR; + case 1: return WARN; + case 2: return INFO; + case 3: return DEBUG; + case 4: return TRACE; + default: return DEFAULT; + } + } } @Getter private static Level loggerLevel; private static void initLogger(@NonNull Level level, String fileName) { - loggerLevel = Level.values()[initInternal(level.getLevel(), fileName)]; + loggerLevel = Level.fromInt(initInternal(level.getLevel(), fileName)); } private static void initLogger(String fileName) { diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index d0004f4dee..59616bda90 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -12,18 +12,18 @@ public class LoggerTests { @Test public void init_logger() { Logger.init(DEFAULT_TEST_LOG_LEVEL); - assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + assertEquals(DEFAULT_TEST_LOG_LEVEL, Logger.getLoggerLevel()); // The logger is already configured, so calling init again shouldn't modify the log level Logger.init(Logger.Level.ERROR); - assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + assertEquals(DEFAULT_TEST_LOG_LEVEL, Logger.getLoggerLevel()); } @Test public void set_logger_config() { Logger.setLoggerConfig(Logger.Level.INFO); - assertEquals(Logger.getLoggerLevel(), Logger.Level.INFO); + assertEquals(Logger.Level.INFO, Logger.getLoggerLevel()); // Revert to the default test log level Logger.setLoggerConfig(DEFAULT_TEST_LOG_LEVEL); - assertEquals(Logger.getLoggerLevel(), DEFAULT_TEST_LOG_LEVEL); + assertEquals(DEFAULT_TEST_LOG_LEVEL, Logger.getLoggerLevel()); } } From 2c3a1e4de65d2815f4d9eed84b6139caedf44261 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 11:39:46 -0700 Subject: [PATCH 10/33] Address Yury's PR comments --- .../main/java/glide/api/logging/Logger.java | 8 ++-- .../ConnectionWithGlideMockTests.java | 4 -- .../src/test/java/glide/LoggerTests.java | 44 +++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index fc2b033aa4..353db0f342 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -6,9 +6,11 @@ /** * A singleton class that allows logging which is consistent with logs from the internal rust core. - * The logger can be set up in 2 ways -
- * 1. By calling Logger.init, which configures the logger only if it wasn't previously configured.
- * 2. By calling Logger.setLoggerConfig, which replaces the existing configuration, and means that new logs will not be + * The logger can be set up in 2 ways - + *
    + *
  1. By calling Logger.init, which configures the logger only if it wasn't previously configured.
    2. + *
  2. By calling Logger.setLoggerConfig, which replaces the existing configuration, and means that new logs will not be
    3. + *
* saved with the logs that were sent before the call.

* If setLoggerConfig wasn't called, the first log attempt will initialize a new logger with default configuration decided * by the Rust core. diff --git a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java index 8fb3653e3b..08235ac1fc 100644 --- a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java +++ b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java @@ -7,12 +7,10 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.any; import connection_request.ConnectionRequestOuterClass.ConnectionRequest; import connection_request.ConnectionRequestOuterClass.NodeAddress; import glide.api.RedisClient; -import glide.api.logging.Logger; import glide.api.models.exceptions.ClosingException; import glide.connectors.handlers.CallbackDispatcher; import glide.connectors.handlers.ChannelHandler; @@ -29,8 +27,6 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.MockedStatic; -import org.mockito.Mockito; import redis_request.RedisRequestOuterClass.RedisRequest; import response.ResponseOuterClass.Response; diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index 59616bda90..50e4fe052b 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -1,10 +1,17 @@ package glide; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import glide.api.logging.Logger; +import lombok.SneakyThrows; import org.junit.jupiter.api.Test; +import java.io.File; +import java.util.Scanner; + public class LoggerTests { private final Logger.Level DEFAULT_TEST_LOG_LEVEL = Logger.Level.WARN; @@ -26,4 +33,41 @@ public void set_logger_config() { Logger.setLoggerConfig(DEFAULT_TEST_LOG_LEVEL); assertEquals(DEFAULT_TEST_LOG_LEVEL, Logger.getLoggerLevel()); } + + @SneakyThrows + @Test + public void log_to_file() { + String infoIdentifier = "Info"; + String infoMessage = "foo"; + String warnIdentifier = "Warn"; + String warnMessage = "woof"; + String errorIdentifier = "Error"; + String errorMessage = "meow"; + String debugIdentifier = "Debug"; + String debugMessage = "chirp"; + String traceIdentifier = "Trace"; + String traceMessage = "squawk"; + + Logger.init(Logger.Level.INFO, "log.txt"); + Logger.log(Logger.Level.INFO, infoIdentifier, infoMessage); + Logger.log(Logger.Level.WARN, warnIdentifier, warnMessage); + Logger.log(Logger.Level.ERROR, errorIdentifier, errorMessage); + Logger.log(Logger.Level.DEBUG, debugIdentifier, debugMessage); + Logger.log(Logger.Level.TRACE, traceIdentifier, traceMessage); + + File logFolder = new File("glide-logs"); + File[] logFiles = logFolder.listFiles((dir, name) -> name.startsWith("log.txt.")); + assertNotNull(logFiles); + File logFile = logFiles[0]; + logFile.deleteOnExit(); + Scanner reader = new Scanner(logFile); + String infoLine = reader.nextLine(); + assertTrue(infoLine.contains(infoIdentifier + " - " + infoMessage)); + String warnLine = reader.nextLine(); + assertTrue(warnLine.contains(warnIdentifier + " - " + warnMessage)); + String errorLine = reader.nextLine(); + assertTrue(errorLine.contains(errorIdentifier + " - " + errorMessage)); + assertFalse(reader.hasNextLine()); + reader.close(); + } } From cba7ad2140d690417eea76953716a098a913a886 Mon Sep 17 00:00:00 2001 From: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com> Date: Thu, 16 May 2024 11:43:46 -0700 Subject: [PATCH 11/33] Update java/src/lib.rs Co-authored-by: Andrew Carbonetto --- java/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 71dd834803..63bbb142c0 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -236,7 +236,7 @@ pub extern "system" fn Java_glide_api_logging_Logger_initInternal<'local>( level: jint, file_name: JString<'local>, ) -> jint { - let level = if level > 0 { Some(level) } else { None }; + let level = if level >= 0 { Some(level) } else { None }; let file_name: Option = match env.get_string(&file_name) { Ok(file_name) => Some(file_name.into()), Err(JniError::NullPtr(_)) => None, From a7062cd561c55eb09d7602d648d7cbcef16a407f Mon Sep 17 00:00:00 2001 From: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com> Date: Thu, 16 May 2024 11:46:45 -0700 Subject: [PATCH 12/33] Apply suggestions from code review Co-authored-by: Andrew Carbonetto --- .../src/main/java/glide/api/logging/Logger.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 353db0f342..42562808be 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -103,7 +103,7 @@ public static void init(String fileName) { * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to * replace an existing logger. * The logger will filter all logs with a level lower than the default level decided by the Rust core. - * The logs will be written to the console. + * The logs will be written to stdout. */ public static void init() { init(Level.DEFAULT, null); @@ -113,7 +113,7 @@ public static void init() { * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to * replace an existing logger. * The logger will filter all logs with a level lower than the given level. - * The logs will be written to the console. + * The logs will be written to stdout. * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. @@ -139,14 +139,14 @@ public static void log( * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to the console. + * Otherwise, logs will be printed to stdout. */ public static void setLoggerConfig(@NonNull Level level, String fileName) { initLogger(level, fileName); } /** - * Creates a new logger instance and configure it with the provided log level and file name. + * Creates a new logger instance and configure it with the provided log level and to stdout. * The logs will be written to the console. * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. @@ -157,11 +157,11 @@ public static void setLoggerConfig(@NonNull Level level) { } /** - * Creates a new logger instance and configure it with the provided log level and file name. + * Creates a new logger instance and configure it with the provided file name and default log level. * The logger will filter all logs with a level lower than the default level decided by the Rust core. * * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to the console. + * Otherwise, logs will be printed to stdout. */ public static void setLoggerConfig(String fileName) { setLoggerConfig(Level.DEFAULT, fileName); @@ -170,7 +170,7 @@ public static void setLoggerConfig(String fileName) { /** * Creates a new logger instance and configure it with the provided log level and file name. * The logger will filter all logs with a level lower than the default level decided by the Rust core. - * The logs will be written to the console. + * The logs will be written to stdout. */ public static void setLoggerConfig() { setLoggerConfig(Level.DEFAULT, null); From 3a13ee24f2f1aaa8afc8d2e800852997b2bf6b46 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 11:53:48 -0700 Subject: [PATCH 13/33] Update docs and import Logger.Level --- java/client/src/main/java/glide/api/logging/Logger.java | 8 ++++---- .../glide/connectors/handlers/CallbackDispatcher.java | 4 +++- .../main/java/glide/connectors/handlers/ReadHandler.java | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 42562808be..83e6c4588f 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -146,8 +146,8 @@ public static void setLoggerConfig(@NonNull Level level, String fileName) { } /** - * Creates a new logger instance and configure it with the provided log level and to stdout. - * The logs will be written to the console. + * Creates a new logger instance and configure it with the provided log level. + * The logs will be written to stdout. * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. @@ -157,7 +157,7 @@ public static void setLoggerConfig(@NonNull Level level) { } /** - * Creates a new logger instance and configure it with the provided file name and default log level. + * Creates a new logger instance and configure it with the provided file name and default log level. * The logger will filter all logs with a level lower than the default level decided by the Rust core. * * @param fileName If provided, the target of the logs will be the file mentioned. @@ -168,7 +168,7 @@ public static void setLoggerConfig(String fileName) { } /** - * Creates a new logger instance and configure it with the provided log level and file name. + * Creates a new logger instance. * The logger will filter all logs with a level lower than the default level decided by the Rust core. * The logs will be written to stdout. */ diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index 30398cc9c7..9373eb0be7 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -18,6 +18,8 @@ import response.ResponseOuterClass.RequestError; import response.ResponseOuterClass.Response; +import static glide.api.logging.Logger.Level.ERROR; + /** Holder for resources required to dispatch responses and used by {@link ReadHandler}. */ @RequiredArgsConstructor public class CallbackDispatcher { @@ -109,7 +111,7 @@ public void completeRequest(Response response) { future.completeAsync(() -> response); } else { // probably a response was received after shutdown or `registerRequest` call was missing - Logger.log(Logger.Level.ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError()); + Logger.log(ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError()); distributeClosingException("Client is in an erroneous state and should close"); } } diff --git a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java index 389889e034..2b2d7f0e24 100644 --- a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java @@ -8,6 +8,8 @@ import lombok.RequiredArgsConstructor; import response.ResponseOuterClass.Response; +import static glide.api.logging.Logger.Level.ERROR; + /** Handler for inbound traffic though UDS. Used by Netty. */ @RequiredArgsConstructor public class ReadHandler extends ChannelInboundHandlerAdapter { @@ -30,7 +32,7 @@ public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg) /** Handles uncaught exceptions from {@link #channelRead(ChannelHandlerContext, Object)}. */ @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - Logger.log(Logger.Level.ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause); + Logger.log(ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause); callbackDispatcher.distributeClosingException( "An unhandled error while reading from UDS channel: " + cause); From 6bf8fede926db94591daf6e1725c5a5db913ed20 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 17:10:25 -0700 Subject: [PATCH 14/33] Split out native function definitions to a new resolver --- .../src/main/java/glide/api/logging/Logger.java | 7 +++---- .../java/glide/ffi/resolvers/LoggerResolver.java | 13 +++++++++++++ java/src/lib.rs | 4 ++-- 3 files changed, 18 insertions(+), 6 deletions(-) create mode 100644 java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 83e6c4588f..86f3e9dd57 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -4,6 +4,9 @@ import lombok.Getter; import lombok.NonNull; +import static glide.ffi.resolvers.LoggerResolver.initInternal; +import static glide.ffi.resolvers.LoggerResolver.logInternal; + /** * A singleton class that allows logging which is consistent with logs from the internal rust core. * The logger can be set up in 2 ways - @@ -175,8 +178,4 @@ public static void setLoggerConfig(String fileName) { public static void setLoggerConfig() { setLoggerConfig(Level.DEFAULT, null); } - - private static native int initInternal(int level, String fileName); - - private static native void logInternal(int level, String logIdentifier, String message); } diff --git a/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java new file mode 100644 index 0000000000..99c91c44e8 --- /dev/null +++ b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java @@ -0,0 +1,13 @@ +/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ +package glide.ffi.resolvers; + +public class LoggerResolver { + // TODO: consider lazy loading the glide_rs library + static { + System.loadLibrary("glide_rs"); + } + + public static native int initInternal(int level, String fileName); + + public static native void logInternal(int level, String logIdentifier, String message); +} diff --git a/java/src/lib.rs b/java/src/lib.rs index 63bbb142c0..343c4327a3 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -207,7 +207,7 @@ impl TryFrom for logger_core::Level { } #[no_mangle] -pub extern "system" fn Java_glide_api_logging_Logger_logInternal<'local>( +pub extern "system" fn Java_glide_ffi_resolvers_LoggerResolver_logInternal<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, level: jint, @@ -230,7 +230,7 @@ pub extern "system" fn Java_glide_api_logging_Logger_logInternal<'local>( } #[no_mangle] -pub extern "system" fn Java_glide_api_logging_Logger_initInternal<'local>( +pub extern "system" fn Java_glide_ffi_resolvers_LoggerResolver_initInternal<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, level: jint, From 01e6b0907cdc9cc8658dcbb9526db5cac0c371b3 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 17:18:30 -0700 Subject: [PATCH 15/33] Remove static loader --- java/client/src/main/java/glide/api/logging/Logger.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 86f3e9dd57..301ab23052 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -19,11 +19,6 @@ * by the Rust core. */ public final class Logger { - // TODO: consider lazy loading the glide_rs library - static { - System.loadLibrary("glide_rs"); - } - @Getter public enum Level { DEFAULT(-1), From 703524398dd0b9af4e3bff7e6a0ca2786e11beb6 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 17:18:59 -0700 Subject: [PATCH 16/33] Run Spotless --- .../main/java/glide/api/logging/Logger.java | 125 ++++++++++-------- .../handlers/CallbackDispatcher.java | 12 +- .../connectors/handlers/ReadHandler.java | 4 +- .../src/test/java/glide/LoggerTests.java | 6 +- 4 files changed, 83 insertions(+), 64 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 301ab23052..11d475f6dd 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -1,22 +1,27 @@ /** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.api.logging; -import lombok.Getter; -import lombok.NonNull; - import static glide.ffi.resolvers.LoggerResolver.initInternal; import static glide.ffi.resolvers.LoggerResolver.logInternal; +import lombok.Getter; +import lombok.NonNull; + /** * A singleton class that allows logging which is consistent with logs from the internal rust core. * The logger can be set up in 2 ways - + * *
    - *
  1. By calling Logger.init, which configures the logger only if it wasn't previously configured.
    2. - *
  2. By calling Logger.setLoggerConfig, which replaces the existing configuration, and means that new logs will not be
    3. + *
  3. By calling Logger.init, which configures the logger only if it wasn't + * previously configured. + *
  4. By calling Logger.setLoggerConfig, which replaces the existing configuration, + * and means that new logs will not be *
- * saved with the logs that were sent before the call.

- * If setLoggerConfig wasn't called, the first log attempt will initialize a new logger with default configuration decided - * by the Rust core. + * + * saved with the logs that were sent before the call.
+ *
+ * If setLoggerConfig wasn't called, the first log attempt will initialize a new logger + * with default configuration decided by the Rust core. */ public final class Logger { @Getter @@ -36,18 +41,23 @@ public enum Level { public static Level fromInt(int i) { switch (i) { - case 0: return ERROR; - case 1: return WARN; - case 2: return INFO; - case 3: return DEBUG; - case 4: return TRACE; - default: return DEFAULT; + case 0: + return ERROR; + case 1: + return WARN; + case 2: + return INFO; + case 3: + return DEBUG; + case 4: + return TRACE; + default: + return DEFAULT; } } } - @Getter - private static Level loggerLevel; + @Getter private static Level loggerLevel; private static void initLogger(@NonNull Level level, String fileName) { loggerLevel = Level.fromInt(initInternal(level.getLevel(), fileName)); @@ -66,16 +76,17 @@ private static void initLogger() { } /** - * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to - * replace an existing logger. - * The logger will filter all logs with a level lower than the given level. - * If given a fileName argument, will write the logs to files postfixed with fileName. If fileName isn't provided, + * Initialize a logger if it wasn't initialized before - this method is meant to be used when + * there is no intention to replace an existing logger. The logger will filter all logs with a + * level lower than the given level. If given a fileName argument, will write the + * logs to files postfixed with fileName. If fileName isn't provided, * the logs will be written to the console. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. - * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. - * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to the console. + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * . If log level isn't provided, the logger will be configured with default + * configuration decided by the Rust core. + * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs + * will be printed to the console. */ public static void init(@NonNull Level level, String fileName) { if (loggerLevel == null) { @@ -84,37 +95,37 @@ public static void init(@NonNull Level level, String fileName) { } /** - * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to - * replace an existing logger. - * The logger will filter all logs with a level lower than the default level decided by the Rust core. - * If given a fileName argument, will write the logs to files postfixed with fileName. If fileName isn't provided, - * the logs will be written to the console. + * Initialize a logger if it wasn't initialized before - this method is meant to be used when + * there is no intention to replace an existing logger. The logger will filter all logs with a + * level lower than the default level decided by the Rust core. If given a fileName + * argument, will write the logs to files postfixed with fileName. If fileName + * isn't provided, the logs will be written to the console. * - * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to the console. + * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs + * will be printed to the console. */ public static void init(String fileName) { init(Level.DEFAULT, fileName); } /** - * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to - * replace an existing logger. - * The logger will filter all logs with a level lower than the default level decided by the Rust core. - * The logs will be written to stdout. + * Initialize a logger if it wasn't initialized before - this method is meant to be used when + * there is no intention to replace an existing logger. The logger will filter all logs with a + * level lower than the default level decided by the Rust core. The logs will be written to + * stdout. */ public static void init() { init(Level.DEFAULT, null); } /** - * Initialize a logger if it wasn't initialized before - this method is meant to be used when there is no intention to - * replace an existing logger. - * The logger will filter all logs with a level lower than the given level. - * The logs will be written to stdout. + * Initialize a logger if it wasn't initialized before - this method is meant to be used when + * there is no intention to replace an existing logger. The logger will filter all logs with a + * level lower than the given level. The logs will be written to stdout. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. - * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * . If log level isn't provided, the logger will be configured with default + * configuration decided by the Rust core. */ public static void init(@NonNull Level level) { init(level, null); @@ -134,41 +145,43 @@ public static void log( /** * Creates a new logger instance and configure it with the provided log level and file name. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. - * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. - * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to stdout. + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * . If log level isn't provided, the logger will be configured with default + * configuration decided by the Rust core. + * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs + * will be printed to stdout. */ public static void setLoggerConfig(@NonNull Level level, String fileName) { initLogger(level, fileName); } /** - * Creates a new logger instance and configure it with the provided log level. - * The logs will be written to stdout. + * Creates a new logger instance and configure it with the provided log level. The logs will be + * written to stdout. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE]. - * If log level isn't provided, the logger will be configured with default configuration decided by the Rust core. + * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * . If log level isn't provided, the logger will be configured with default + * configuration decided by the Rust core. */ public static void setLoggerConfig(@NonNull Level level) { setLoggerConfig(level, null); } /** - * Creates a new logger instance and configure it with the provided file name and default log level. - * The logger will filter all logs with a level lower than the default level decided by the Rust core. + * Creates a new logger instance and configure it with the provided file name and default log + * level. The logger will filter all logs with a level lower than the default level decided by the + * Rust core. * - * @param fileName If provided, the target of the logs will be the file mentioned. - * Otherwise, logs will be printed to stdout. + * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs + * will be printed to stdout. */ public static void setLoggerConfig(String fileName) { setLoggerConfig(Level.DEFAULT, fileName); } /** - * Creates a new logger instance. - * The logger will filter all logs with a level lower than the default level decided by the Rust core. - * The logs will be written to stdout. + * Creates a new logger instance. The logger will filter all logs with a level lower than the + * default level decided by the Rust core. The logs will be written to stdout. */ public static void setLoggerConfig() { setLoggerConfig(Level.DEFAULT, null); diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index 9373eb0be7..f383110065 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -1,6 +1,8 @@ /** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.connectors.handlers; +import static glide.api.logging.Logger.Level.ERROR; + import glide.api.logging.Logger; import glide.api.models.exceptions.ClosingException; import glide.api.models.exceptions.ConnectionException; @@ -18,8 +20,6 @@ import response.ResponseOuterClass.RequestError; import response.ResponseOuterClass.Response; -import static glide.api.logging.Logger.Level.ERROR; - /** Holder for resources required to dispatch responses and used by {@link ReadHandler}. */ @RequiredArgsConstructor public class CallbackDispatcher { @@ -111,7 +111,13 @@ public void completeRequest(Response response) { future.completeAsync(() -> response); } else { // probably a response was received after shutdown or `registerRequest` call was missing - Logger.log(ERROR, "callback dispatcher", "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError()); + Logger.log( + ERROR, + "callback dispatcher", + "Received a response for not registered callback id " + + callbackId + + ", request error = " + + response.getRequestError()); distributeClosingException("Client is in an erroneous state and should close"); } } diff --git a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java index 2b2d7f0e24..bb624aa9c5 100644 --- a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java @@ -1,6 +1,8 @@ /** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.connectors.handlers; +import static glide.api.logging.Logger.Level.ERROR; + import glide.api.logging.Logger; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; @@ -8,8 +10,6 @@ import lombok.RequiredArgsConstructor; import response.ResponseOuterClass.Response; -import static glide.api.logging.Logger.Level.ERROR; - /** Handler for inbound traffic though UDS. Used by Netty. */ @RequiredArgsConstructor public class ReadHandler extends ChannelInboundHandlerAdapter { diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index 50e4fe052b..b6ebd7deb1 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -1,3 +1,4 @@ +/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ package glide; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -6,11 +7,10 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import glide.api.logging.Logger; -import lombok.SneakyThrows; -import org.junit.jupiter.api.Test; - import java.io.File; import java.util.Scanner; +import lombok.SneakyThrows; +import org.junit.jupiter.api.Test; public class LoggerTests { From fcb402689f536ed00caf1f3fb0bb0073733fc90f Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 17:27:42 -0700 Subject: [PATCH 17/33] Change references to Rust core to Glide core --- .../src/main/java/glide/api/logging/Logger.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 11d475f6dd..78c4d77c4e 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -21,7 +21,7 @@ * saved with the logs that were sent before the call.
*
* If setLoggerConfig wasn't called, the first log attempt will initialize a new logger - * with default configuration decided by the Rust core. + * with default configuration decided by Glide core. */ public final class Logger { @Getter @@ -84,7 +84,7 @@ private static void initLogger() { * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default - * configuration decided by the Rust core. + * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs * will be printed to the console. */ @@ -97,7 +97,7 @@ public static void init(@NonNull Level level, String fileName) { /** * Initialize a logger if it wasn't initialized before - this method is meant to be used when * there is no intention to replace an existing logger. The logger will filter all logs with a - * level lower than the default level decided by the Rust core. If given a fileName + * level lower than the default level decided by Glide core. If given a fileName * argument, will write the logs to files postfixed with fileName. If fileName * isn't provided, the logs will be written to the console. * @@ -111,7 +111,7 @@ public static void init(String fileName) { /** * Initialize a logger if it wasn't initialized before - this method is meant to be used when * there is no intention to replace an existing logger. The logger will filter all logs with a - * level lower than the default level decided by the Rust core. The logs will be written to + * level lower than the default level decided by Glide core. The logs will be written to * stdout. */ public static void init() { @@ -125,7 +125,7 @@ public static void init() { * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default - * configuration decided by the Rust core. + * configuration decided by Glide core. */ public static void init(@NonNull Level level) { init(level, null); @@ -147,7 +147,7 @@ public static void log( * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default - * configuration decided by the Rust core. + * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs * will be printed to stdout. */ @@ -161,7 +161,7 @@ public static void setLoggerConfig(@NonNull Level level, String fileName) { * * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default - * configuration decided by the Rust core. + * configuration decided by Glide core. */ public static void setLoggerConfig(@NonNull Level level) { setLoggerConfig(level, null); @@ -181,7 +181,7 @@ public static void setLoggerConfig(String fileName) { /** * Creates a new logger instance. The logger will filter all logs with a level lower than the - * default level decided by the Rust core. The logs will be written to stdout. + * default level decided by Glide core. The logs will be written to stdout. */ public static void setLoggerConfig() { setLoggerConfig(Level.DEFAULT, null); From 5094a4ec90e1ca8d70e62974498427253b3d91e3 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 17:36:09 -0700 Subject: [PATCH 18/33] Spotless again --- java/client/src/main/java/glide/api/logging/Logger.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 78c4d77c4e..8860f8773d 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -111,8 +111,7 @@ public static void init(String fileName) { /** * Initialize a logger if it wasn't initialized before - this method is meant to be used when * there is no intention to replace an existing logger. The logger will filter all logs with a - * level lower than the default level decided by Glide core. The logs will be written to - * stdout. + * level lower than the default level decided by Glide core. The logs will be written to stdout. */ public static void init() { init(Level.DEFAULT, null); From 4b4bb44ddbbac7ffa39b8911b207e67b2c54d0aa Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 16 May 2024 20:40:09 -0700 Subject: [PATCH 19/33] Make minor documentation fixes --- java/client/src/main/java/glide/api/logging/Logger.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 8860f8773d..f35051a9ef 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -15,11 +15,9 @@ *
  • By calling Logger.init, which configures the logger only if it wasn't * previously configured. *
  • By calling Logger.setLoggerConfig, which replaces the existing configuration, - * and means that new logs will not be + * and means that new logs will not be saved with the logs that were sent before the call. * * - * saved with the logs that were sent before the call.
    - *
    * If setLoggerConfig wasn't called, the first log attempt will initialize a new logger * with default configuration decided by Glide core. */ @@ -169,7 +167,7 @@ public static void setLoggerConfig(@NonNull Level level) { /** * Creates a new logger instance and configure it with the provided file name and default log * level. The logger will filter all logs with a level lower than the default level decided by the - * Rust core. + * Glide core. * * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs * will be printed to stdout. From 1eccd3c158b049def12f30e2390f74e3f070440c Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Fri, 17 May 2024 15:30:36 -0700 Subject: [PATCH 20/33] Fix failing tests --- .../src/main/java/glide/api/logging/Logger.java | 5 +++++ .../java/glide/ffi/resolvers/LoggerResolver.java | 2 +- .../src/test/java/glide/ExceptionHandlingTests.java | 12 ++++++++++++ .../connection/ConnectionWithGlideMockTests.java | 2 ++ .../src/test/java/glide/utils/RustCoreMock.java | 4 ++++ java/integTest/build.gradle | 1 - java/integTest/src/test/java/glide/LoggerTests.java | 2 +- 7 files changed, 25 insertions(+), 3 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index f35051a9ef..53e4cdfce5 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -24,6 +24,7 @@ public final class Logger { @Getter public enum Level { + DISABLED(-2), DEFAULT(-1), ERROR(0), WARN(1), @@ -58,6 +59,10 @@ public static Level fromInt(int i) { @Getter private static Level loggerLevel; private static void initLogger(@NonNull Level level, String fileName) { + if (level == Level.DISABLED) { + loggerLevel = level; + return; + } loggerLevel = Level.fromInt(initInternal(level.getLevel(), fileName)); } diff --git a/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java index 99c91c44e8..1a9bbc0466 100644 --- a/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java +++ b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java @@ -4,7 +4,7 @@ public class LoggerResolver { // TODO: consider lazy loading the glide_rs library static { - System.loadLibrary("glide_rs"); + NativeUtils.loadGlideLib(); } public static native int initInternal(int level, String fileName); diff --git a/java/client/src/test/java/glide/ExceptionHandlingTests.java b/java/client/src/test/java/glide/ExceptionHandlingTests.java index 43bdb224d0..4c0c5cef02 100644 --- a/java/client/src/test/java/glide/ExceptionHandlingTests.java +++ b/java/client/src/test/java/glide/ExceptionHandlingTests.java @@ -6,6 +6,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mockStatic; import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand; import static response.ResponseOuterClass.RequestErrorType.Disconnect; import static response.ResponseOuterClass.RequestErrorType.ExecAbort; @@ -13,6 +14,7 @@ import static response.ResponseOuterClass.RequestErrorType.Unspecified; import connection_request.ConnectionRequestOuterClass; +import glide.api.logging.Logger; import glide.api.models.configuration.RedisClientConfiguration; import glide.api.models.exceptions.ClosingException; import glide.api.models.exceptions.ConnectionException; @@ -35,6 +37,7 @@ import java.util.stream.Stream; import lombok.RequiredArgsConstructor; import lombok.SneakyThrows; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -44,10 +47,14 @@ import response.ResponseOuterClass.RequestError; import response.ResponseOuterClass.RequestErrorType; import response.ResponseOuterClass.Response; +import org.mockito.MockedStatic; public class ExceptionHandlingTests { + private MockedStatic mockedLogger; + @BeforeEach public void init() { + mockedLogger = mockStatic(Logger.class); var threadPoolResource = ThreadPoolResourceAllocator.getOrCreate(() -> null); if (threadPoolResource != null) { threadPoolResource.getEventLoopGroup().shutdownGracefully(); @@ -55,6 +62,11 @@ public void init() { } } + @AfterEach + public void teardown() { + mockedLogger.close(); + } + /** * This test shows how exception handling works in the middle of future pipeline The client has * similar stuff, but it rethrows an exception. diff --git a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java index 08235ac1fc..26ec052a13 100644 --- a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java +++ b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java @@ -11,6 +11,7 @@ import connection_request.ConnectionRequestOuterClass.ConnectionRequest; import connection_request.ConnectionRequestOuterClass.NodeAddress; import glide.api.RedisClient; +import glide.api.logging.Logger; import glide.api.models.exceptions.ClosingException; import glide.connectors.handlers.CallbackDispatcher; import glide.connectors.handlers.ChannelHandler; @@ -37,6 +38,7 @@ public class ConnectionWithGlideMockTests extends RustCoreLibMockTestBase { @BeforeEach @SneakyThrows public void createTestClient() { + Logger.setLoggerConfig(Logger.Level.DISABLED); channelHandler = new ChannelHandler( new CallbackDispatcher(), socketPath, Platform.getThreadPoolResourceSupplier().get()); diff --git a/java/client/src/test/java/glide/utils/RustCoreMock.java b/java/client/src/test/java/glide/utils/RustCoreMock.java index b9bc53bae6..af9702ce6a 100644 --- a/java/client/src/test/java/glide/utils/RustCoreMock.java +++ b/java/client/src/test/java/glide/utils/RustCoreMock.java @@ -2,6 +2,7 @@ package glide.utils; import connection_request.ConnectionRequestOuterClass.ConnectionRequest; +import glide.api.logging.Logger; import glide.connectors.resources.Platform; import io.netty.bootstrap.ServerBootstrap; import io.netty.buffer.ByteBuf; @@ -24,10 +25,13 @@ import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.SneakyThrows; +import org.mockito.MockedStatic; import redis_request.RedisRequestOuterClass.RedisRequest; import response.ResponseOuterClass.ConstantResponse; import response.ResponseOuterClass.Response; +import static org.mockito.Mockito.mockStatic; + public class RustCoreMock { @FunctionalInterface diff --git a/java/integTest/build.gradle b/java/integTest/build.gradle index f17d5d8ac8..d955262c08 100644 --- a/java/integTest/build.gradle +++ b/java/integTest/build.gradle @@ -134,7 +134,6 @@ tasks.withType(Test) { events "started", "skipped", "passed", "failed" showStandardStreams true } - jvmArgs "-Djava.library.path=${project.rootDir}/target/release" afterTest { desc, result -> logger.quiet "${desc.className}.${desc.name}: ${result.resultType} ${(result.getEndTime() - result.getStartTime())/1000}s" } diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index b6ebd7deb1..c4844a20e7 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -48,7 +48,7 @@ public void log_to_file() { String traceIdentifier = "Trace"; String traceMessage = "squawk"; - Logger.init(Logger.Level.INFO, "log.txt"); + Logger.setLoggerConfig(Logger.Level.INFO, "log.txt"); Logger.log(Logger.Level.INFO, infoIdentifier, infoMessage); Logger.log(Logger.Level.WARN, warnIdentifier, warnMessage); Logger.log(Logger.Level.ERROR, errorIdentifier, errorMessage); From 0cfd92bea15f96fd695786830e3b2a7b74975b6b Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Fri, 17 May 2024 15:31:08 -0700 Subject: [PATCH 21/33] Run spotless --- java/client/src/test/java/glide/ExceptionHandlingTests.java | 2 +- java/client/src/test/java/glide/utils/RustCoreMock.java | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/java/client/src/test/java/glide/ExceptionHandlingTests.java b/java/client/src/test/java/glide/ExceptionHandlingTests.java index 4c0c5cef02..c6706855c6 100644 --- a/java/client/src/test/java/glide/ExceptionHandlingTests.java +++ b/java/client/src/test/java/glide/ExceptionHandlingTests.java @@ -43,11 +43,11 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.MockedStatic; import redis_request.RedisRequestOuterClass.RedisRequest; import response.ResponseOuterClass.RequestError; import response.ResponseOuterClass.RequestErrorType; import response.ResponseOuterClass.Response; -import org.mockito.MockedStatic; public class ExceptionHandlingTests { private MockedStatic mockedLogger; diff --git a/java/client/src/test/java/glide/utils/RustCoreMock.java b/java/client/src/test/java/glide/utils/RustCoreMock.java index af9702ce6a..b9bc53bae6 100644 --- a/java/client/src/test/java/glide/utils/RustCoreMock.java +++ b/java/client/src/test/java/glide/utils/RustCoreMock.java @@ -2,7 +2,6 @@ package glide.utils; import connection_request.ConnectionRequestOuterClass.ConnectionRequest; -import glide.api.logging.Logger; import glide.connectors.resources.Platform; import io.netty.bootstrap.ServerBootstrap; import io.netty.buffer.ByteBuf; @@ -25,13 +24,10 @@ import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.SneakyThrows; -import org.mockito.MockedStatic; import redis_request.RedisRequestOuterClass.RedisRequest; import response.ResponseOuterClass.ConstantResponse; import response.ResponseOuterClass.Response; -import static org.mockito.Mockito.mockStatic; - public class RustCoreMock { @FunctionalInterface From 8c424355c7088477078a457045b8821b3410ec12 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Wed, 12 Jun 2024 14:08:32 -0700 Subject: [PATCH 22/33] Address PR comments --- .../main/java/glide/api/logging/Logger.java | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 53e4cdfce5..31dea16bd3 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -66,18 +66,6 @@ private static void initLogger(@NonNull Level level, String fileName) { loggerLevel = Level.fromInt(initInternal(level.getLevel(), fileName)); } - private static void initLogger(String fileName) { - initLogger(Level.DEFAULT, fileName); - } - - private static void initLogger(@NonNull Level level) { - initLogger(level, null); - } - - private static void initLogger() { - initLogger(Level.DEFAULT, null); - } - /** * Initialize a logger if it wasn't initialized before - this method is meant to be used when * there is no intention to replace an existing logger. The logger will filter all logs with a @@ -85,7 +73,7 @@ private static void initLogger() { * logs to files postfixed with fileName. If fileName isn't provided, * the logs will be written to the console. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs @@ -104,10 +92,10 @@ public static void init(@NonNull Level level, String fileName) { * argument, will write the logs to files postfixed with fileName. If fileName * isn't provided, the logs will be written to the console. * - * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs + * @param fileName The target of the logs will be the file mentioned. Otherwise, logs * will be printed to the console. */ - public static void init(String fileName) { + public static void init(@NonNull String fileName) { init(Level.DEFAULT, fileName); } @@ -133,6 +121,13 @@ public static void init(@NonNull Level level) { init(level, null); } + /** + * Logs the provided message if the provided log level is lower then the logger level. + * + * @param level The log level of the provided message. + * @param logIdentifier The log identifier should give the log a context. + * @param message The message to log. + */ public static void log( @NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { if (loggerLevel == null) { @@ -147,7 +142,7 @@ public static void log( /** * Creates a new logger instance and configure it with the provided log level and file name. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs @@ -161,7 +156,7 @@ public static void setLoggerConfig(@NonNull Level level, String fileName) { * Creates a new logger instance and configure it with the provided log level. The logs will be * written to stdout. * - * @param level Set the logger level to one of [DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. */ From 6c9b7aef21332295a17ab0a936d2bf742d43b063 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Wed, 12 Jun 2024 14:14:02 -0700 Subject: [PATCH 23/33] Apply Spotless --- .../src/main/java/glide/api/logging/Logger.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 31dea16bd3..7794d7ea77 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -73,7 +73,8 @@ private static void initLogger(@NonNull Level level, String fileName) { * logs to files postfixed with fileName. If fileName isn't provided, * the logs will be written to the console. * - * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of + * [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs @@ -92,8 +93,8 @@ public static void init(@NonNull Level level, String fileName) { * argument, will write the logs to files postfixed with fileName. If fileName * isn't provided, the logs will be written to the console. * - * @param fileName The target of the logs will be the file mentioned. Otherwise, logs - * will be printed to the console. + * @param fileName The target of the logs will be the file mentioned. Otherwise, logs will be + * printed to the console. */ public static void init(@NonNull String fileName) { init(Level.DEFAULT, fileName); @@ -142,7 +143,8 @@ public static void log( /** * Creates a new logger instance and configure it with the provided log level and file name. * - * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of + * [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. * @param fileName If provided, the target of the logs will be the file mentioned. Otherwise, logs @@ -156,7 +158,8 @@ public static void setLoggerConfig(@NonNull Level level, String fileName) { * Creates a new logger instance and configure it with the provided log level. The logs will be * written to stdout. * - * @param level Set the logger level to one of [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] + * @param level Set the logger level to one of + * [DISABLED, DEFAULT, ERROR, WARN, INFO, DEBUG, TRACE] * . If log level isn't provided, the logger will be configured with default * configuration decided by Glide core. */ From 23c9957d5dd3322c8858474f2249feaeb70d7fee Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Wed, 12 Jun 2024 14:19:43 -0700 Subject: [PATCH 24/33] Minor documentation fixes --- java/client/src/main/java/glide/api/logging/Logger.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 7794d7ea77..349b5fb81c 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -89,12 +89,10 @@ public static void init(@NonNull Level level, String fileName) { /** * Initialize a logger if it wasn't initialized before - this method is meant to be used when * there is no intention to replace an existing logger. The logger will filter all logs with a - * level lower than the default level decided by Glide core. If given a fileName - * argument, will write the logs to files postfixed with fileName. If fileName - * isn't provided, the logs will be written to the console. + * level lower than the default level decided by Glide core. Given a fileName + * argument, will write the logs to files postfixed with fileName. * - * @param fileName The target of the logs will be the file mentioned. Otherwise, logs will be - * printed to the console. + * @param fileName The target of the logs will be the file mentioned. */ public static void init(@NonNull String fileName) { init(Level.DEFAULT, fileName); From b8515986676654bf093b7b5e773bc54e7d9676d3 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 27 Jun 2024 17:29:50 -0700 Subject: [PATCH 25/33] Don't log when DISABLED log level is passed to log method --- java/client/src/main/java/glide/api/logging/Logger.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 349b5fb81c..a2cc042e76 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -132,6 +132,11 @@ public static void log( if (loggerLevel == null) { initLogger(Level.DEFAULT, null); } + + if (level == Level.DISABLED) { + return; + } + if (!(level.getLevel() <= loggerLevel.getLevel())) { return; } From 593702e6f0dede62b179c902070a9022a7708114 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sun, 30 Jun 2024 21:20:27 -0700 Subject: [PATCH 26/33] Fix build errors and handle errors in FFI layer --- java/src/errors.rs | 2 ++ java/src/lib.rs | 29 +++++++++++++++++++---------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/java/src/errors.rs b/java/src/errors.rs index d4d58ac554..93372a7257 100644 --- a/java/src/errors.rs +++ b/java/src/errors.rs @@ -9,6 +9,7 @@ pub enum FFIError { Jni(JNIError), Uds(String), Utf8(FromUtf8Error), + Logger(String), } impl From for FFIError { @@ -29,6 +30,7 @@ impl std::fmt::Display for FFIError { FFIError::Jni(err) => write!(f, "{}", err), FFIError::Uds(err) => write!(f, "{}", err), FFIError::Utf8(err) => write!(f, "{}", err), + FFIError::Logger(err) => write!(f, "{}", err), } } } diff --git a/java/src/lib.rs b/java/src/lib.rs index e70104f081..54ac731f10 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -5,8 +5,9 @@ use glide_core::start_socket_listener as start_socket_listener_core; use glide_core::MAX_REQUEST_ARGS_LENGTH as MAX_REQUEST_ARGS_LENGTH_IN_BYTES; use bytes::Bytes; +use jni::errors::Error as JniError; use jni::objects::{JByteArray, JClass, JObject, JObjectArray, JString}; -use jni::sys::{jlong, jsize}; +use jni::sys::{jint, jlong, jsize}; use jni::JNIEnv; use redis::Value; use std::sync::mpsc; @@ -293,7 +294,7 @@ impl From for Level { } impl TryFrom for logger_core::Level { - type Error = String; + type Error = FFIError; fn try_from(level: Level) -> Result>::Error> { match level.0 { 0 => Ok(logger_core::Level::Error), @@ -301,7 +302,10 @@ impl TryFrom for logger_core::Level { 2 => Ok(logger_core::Level::Info), 3 => Ok(logger_core::Level::Debug), 4 => Ok(logger_core::Level::Trace), - _ => Err(format!("Invalid log level: {:?}", level.0)), + _ => Err(FFIError::Logger(format!( + "Invalid log level: {:?}", + level.0 + ))), } } } @@ -348,20 +352,25 @@ pub extern "system" fn Java_glide_ffi_resolvers_LoggerResolver_initInternal<'loc ) -> jint { handle_panics( move || { - fn init_internal(level: jint, file_name: JString<'_>) -> Result { + fn init_internal( + env: &mut JNIEnv<'_>, + level: jint, + file_name: JString<'_>, + ) -> Result { let level = if level >= 0 { Some(level) } else { None }; let file_name: Option = match env.get_string(&file_name) { Ok(file_name) => Some(file_name.into()), Err(JniError::NullPtr(_)) => None, - Err(err) => return err, + Err(err) => return Err(err.into()), + }; + let level = match level { + Some(lvl) => Some(Level(lvl).try_into()?), + None => None, }; - let logger_level = logger_core::init( - level.map(|level| Level(level).try_into()?), - file_name.as_deref(), - ); + let logger_level = logger_core::init(level, file_name.as_deref()); Ok(Level::from(logger_level).0) } - let result = init_internal(level, file_name); + let result = init_internal(&mut env, level, file_name); handle_errors(&mut env, result) }, "initInternal", From 9e75012a3c4f3a9cc722b4fadc32b91912c9c034 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sun, 30 Jun 2024 21:38:42 -0700 Subject: [PATCH 27/33] Make logger messages lazily evaluated --- .../client/src/main/java/glide/api/logging/Logger.java | 6 ++++-- .../glide/connectors/handlers/CallbackDispatcher.java | 2 +- .../java/glide/connectors/handlers/ReadHandler.java | 2 +- java/integTest/src/test/java/glide/LoggerTests.java | 10 +++++----- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index a2cc042e76..8a520c07bc 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -7,6 +7,8 @@ import lombok.Getter; import lombok.NonNull; +import java.util.function.Supplier; + /** * A singleton class that allows logging which is consistent with logs from the internal rust core. * The logger can be set up in 2 ways - @@ -128,7 +130,7 @@ public static void init(@NonNull Level level) { * @param message The message to log. */ public static void log( - @NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { + @NonNull Level level, @NonNull String logIdentifier, @NonNull Supplier message) { if (loggerLevel == null) { initLogger(Level.DEFAULT, null); } @@ -140,7 +142,7 @@ public static void log( if (!(level.getLevel() <= loggerLevel.getLevel())) { return; } - logInternal(level.getLevel(), logIdentifier, message); + logInternal(level.getLevel(), logIdentifier, message.get()); } /** diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index 708b177ef9..86daf8d4ef 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -114,7 +114,7 @@ public void completeRequest(Response response) { Logger.log( ERROR, "callback dispatcher", - "Received a response for not registered callback id " + () -> "Received a response for not registered callback id " + callbackId + ", request error = " + response.getRequestError()); diff --git a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java index f6773e621d..d78d144cea 100644 --- a/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/ReadHandler.java @@ -32,7 +32,7 @@ public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg) /** Handles uncaught exceptions from {@link #channelRead(ChannelHandlerContext, Object)}. */ @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { - Logger.log(ERROR, "read handler", "=== exceptionCaught " + ctx + " " + cause); + Logger.log(ERROR, "read handler", () -> "=== exceptionCaught " + ctx + " " + cause); callbackDispatcher.distributeClosingException( "An unhandled error while reading from UDS channel: " + cause); diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index c4844a20e7..78e38a5b36 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -49,11 +49,11 @@ public void log_to_file() { String traceMessage = "squawk"; Logger.setLoggerConfig(Logger.Level.INFO, "log.txt"); - Logger.log(Logger.Level.INFO, infoIdentifier, infoMessage); - Logger.log(Logger.Level.WARN, warnIdentifier, warnMessage); - Logger.log(Logger.Level.ERROR, errorIdentifier, errorMessage); - Logger.log(Logger.Level.DEBUG, debugIdentifier, debugMessage); - Logger.log(Logger.Level.TRACE, traceIdentifier, traceMessage); + Logger.log(Logger.Level.INFO, infoIdentifier, () -> infoMessage); + Logger.log(Logger.Level.WARN, warnIdentifier, () -> warnMessage); + Logger.log(Logger.Level.ERROR, errorIdentifier, () -> errorMessage); + Logger.log(Logger.Level.DEBUG, debugIdentifier, () -> debugMessage); + Logger.log(Logger.Level.TRACE, traceIdentifier, () -> traceMessage); File logFolder = new File("glide-logs"); File[] logFiles = logFolder.listFiles((dir, name) -> name.startsWith("log.txt.")); From 16aaeb0a9073c34fd3fce76fa8f0acd7cc2c34b0 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sun, 30 Jun 2024 22:01:36 -0700 Subject: [PATCH 28/33] Run Spotless --- java/client/src/main/java/glide/api/logging/Logger.java | 5 ++--- .../glide/connectors/handlers/CallbackDispatcher.java | 9 +++++---- .../main/java/glide/ffi/resolvers/LoggerResolver.java | 2 +- java/integTest/src/test/java/glide/LoggerTests.java | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index 8a520c07bc..e3ebb58499 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -1,14 +1,13 @@ -/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ +/** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.api.logging; import static glide.ffi.resolvers.LoggerResolver.initInternal; import static glide.ffi.resolvers.LoggerResolver.logInternal; +import java.util.function.Supplier; import lombok.Getter; import lombok.NonNull; -import java.util.function.Supplier; - /** * A singleton class that allows logging which is consistent with logs from the internal rust core. * The logger can be set up in 2 ways - diff --git a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java index ee89bba483..cfd349afdf 100644 --- a/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java +++ b/java/client/src/main/java/glide/connectors/handlers/CallbackDispatcher.java @@ -122,10 +122,11 @@ public void completeRequest(Response response) { Logger.log( ERROR, "callback dispatcher", - () -> "Received a response for not registered callback id " - + callbackId - + ", request error = " - + response.getRequestError()); + () -> + "Received a response for not registered callback id " + + callbackId + + ", request error = " + + response.getRequestError()); distributeClosingException("Client is in an erroneous state and should close"); } } diff --git a/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java index 1a9bbc0466..4c1e2469cc 100644 --- a/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java +++ b/java/client/src/main/java/glide/ffi/resolvers/LoggerResolver.java @@ -1,4 +1,4 @@ -/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ +/** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.ffi.resolvers; public class LoggerResolver { diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index 78e38a5b36..7506fdd16d 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -1,4 +1,4 @@ -/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ +/** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ package glide; import static org.junit.jupiter.api.Assertions.assertEquals; From fea98cdc71427c1cbc586270a456ba976b09ce83 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 1 Jul 2024 12:29:20 -0700 Subject: [PATCH 29/33] Add overload for log method --- .../main/java/glide/api/logging/Logger.java | 32 +++++++++++++++++-- .../src/test/java/glide/LoggerTests.java | 13 ++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/java/client/src/main/java/glide/api/logging/Logger.java b/java/client/src/main/java/glide/api/logging/Logger.java index e3ebb58499..744c5c2ddc 100644 --- a/java/client/src/main/java/glide/api/logging/Logger.java +++ b/java/client/src/main/java/glide/api/logging/Logger.java @@ -122,14 +122,40 @@ public static void init(@NonNull Level level) { } /** - * Logs the provided message if the provided log level is lower then the logger level. + * Logs the provided message if the provided log level is lower than the logger level. This + * overload takes a Supplier to lazily construct the message. + * + * @param level The log level of the provided message. + * @param logIdentifier The log identifier should give the log a context. + * @param messageSupplier The Supplier of the message to log. + */ + public static void log( + @NonNull Level level, + @NonNull String logIdentifier, + @NonNull Supplier messageSupplier) { + if (loggerLevel == null) { + initLogger(Level.DEFAULT, null); + } + + if (level == Level.DISABLED) { + return; + } + + if (!(level.getLevel() <= loggerLevel.getLevel())) { + return; + } + logInternal(level.getLevel(), logIdentifier, messageSupplier.get()); + } + + /** + * Logs the provided message if the provided log level is lower than the logger level. * * @param level The log level of the provided message. * @param logIdentifier The log identifier should give the log a context. * @param message The message to log. */ public static void log( - @NonNull Level level, @NonNull String logIdentifier, @NonNull Supplier message) { + @NonNull Level level, @NonNull String logIdentifier, @NonNull String message) { if (loggerLevel == null) { initLogger(Level.DEFAULT, null); } @@ -141,7 +167,7 @@ public static void log( if (!(level.getLevel() <= loggerLevel.getLevel())) { return; } - logInternal(level.getLevel(), logIdentifier, message.get()); + logInternal(level.getLevel(), logIdentifier, message); } /** diff --git a/java/integTest/src/test/java/glide/LoggerTests.java b/java/integTest/src/test/java/glide/LoggerTests.java index 7506fdd16d..02c4a35f6d 100644 --- a/java/integTest/src/test/java/glide/LoggerTests.java +++ b/java/integTest/src/test/java/glide/LoggerTests.java @@ -49,6 +49,13 @@ public void log_to_file() { String traceMessage = "squawk"; Logger.setLoggerConfig(Logger.Level.INFO, "log.txt"); + Logger.log(Logger.Level.INFO, infoIdentifier, infoMessage); + Logger.log(Logger.Level.WARN, warnIdentifier, warnMessage); + Logger.log(Logger.Level.ERROR, errorIdentifier, errorMessage); + Logger.log(Logger.Level.DEBUG, debugIdentifier, debugMessage); + Logger.log(Logger.Level.TRACE, traceIdentifier, traceMessage); + + // Test logging with lazily constructed messages Logger.log(Logger.Level.INFO, infoIdentifier, () -> infoMessage); Logger.log(Logger.Level.WARN, warnIdentifier, () -> warnMessage); Logger.log(Logger.Level.ERROR, errorIdentifier, () -> errorMessage); @@ -67,6 +74,12 @@ public void log_to_file() { assertTrue(warnLine.contains(warnIdentifier + " - " + warnMessage)); String errorLine = reader.nextLine(); assertTrue(errorLine.contains(errorIdentifier + " - " + errorMessage)); + String infoLineLazy = reader.nextLine(); + assertTrue(infoLineLazy.contains(infoIdentifier + " - " + infoMessage)); + String warnLineLazy = reader.nextLine(); + assertTrue(warnLineLazy.contains(warnIdentifier + " - " + warnMessage)); + String errorLineLazy = reader.nextLine(); + assertTrue(errorLineLazy.contains(errorIdentifier + " - " + errorMessage)); assertFalse(reader.hasNextLine()); reader.close(); } From ed7fec69753f23d1726bba99476fc007572be672 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 1 Jul 2024 14:36:47 -0700 Subject: [PATCH 30/33] Address Logger TODOs in MessageHandler --- .../connectors/handlers/MessageHandler.java | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java index 74de6ddeb0..35f955488a 100644 --- a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java @@ -1,6 +1,7 @@ /** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ package glide.connectors.handlers; +import glide.api.logging.Logger; import glide.api.models.PubSubMessage; import glide.api.models.configuration.BaseSubscriptionConfiguration.MessageCallback; import glide.api.models.exceptions.RedisException; @@ -39,8 +40,10 @@ public class MessageHandler { public void handle(Response response) { Object data = responseResolver.apply(response); if (!(data instanceof Map)) { - // TODO log thru logger https://github.com/aws/glide-for-redis/pull/1422 - System.err.println("Received invalid push: empty or in incorrect format."); + Logger.log( + Logger.Level.WARN, + "invalid push", + "Received invalid push: empty or in incorrect format."); throw new RedisException("Received invalid push: empty or in incorrect format."); } @SuppressWarnings("unchecked") @@ -50,11 +53,10 @@ public void handle(Response response) { switch (pushType) { case Disconnection: - // TODO log thru logger https://github.com/aws/glide-for-redis/pull/1422 - // ClientLogger.log( - // LogLevel.WARN, - // "disconnect notification", - // "Transport disconnected, messages might be lost", + Logger.log( + Logger.Level.WARN, + "disconnect notification", + "Transport disconnected, messages might be lost"); break; case PMessage: handle(new PubSubMessage((String) values[2], (String) values[1], (String) values[0])); @@ -70,21 +72,21 @@ public void handle(Response response) { case PUnsubscribe: case SUnsubscribe: // ignore for now - // TODO log thru logger https://github.com/aws/glide-for-redis/pull/1422 - System.out.printf( - "Received push notification of type '%s': %s\n", - pushType, - Arrays.stream(values) - .map(v -> v instanceof Number ? v.toString() : String.format("'%s'", v)) - .collect(Collectors.joining(" "))); + Logger.log( + Logger.Level.INFO, + "SUnsubscribe notification", + String.format( + "Received push notification of type '%s': %s\n", + pushType, + Arrays.stream(values) + .map(v -> v instanceof Number ? v.toString() : String.format("'%s'", v)) + .collect(Collectors.joining(" ")))); break; default: - // TODO log thru logger https://github.com/aws/glide-for-redis/pull/1422 - System.err.printf("Received push with unsupported type: %s.\n", pushType); - // ClientLogger.log( - // LogLevel.WARN, - // "unknown notification", - // f"Unknown notification message: '{message_kind}'", + Logger.log( + Logger.Level.WARN, + "unknown notification", + String.format("Unknown notification message: '%s'", pushType)); } } From 1ce4d6083054ffa1f33783e080328d533a2193f2 Mon Sep 17 00:00:00 2001 From: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:54:38 -0700 Subject: [PATCH 31/33] Update java/client/src/main/java/glide/connectors/handlers/MessageHandler.java Co-authored-by: Yury-Fridlyand --- .../main/java/glide/connectors/handlers/MessageHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java index 35f955488a..6cfce1f1e8 100644 --- a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java @@ -74,9 +74,9 @@ public void handle(Response response) { // ignore for now Logger.log( Logger.Level.INFO, - "SUnsubscribe notification", + "subscribe/unsubscribe notification", String.format( - "Received push notification of type '%s': %s\n", + "Received push notification of type '%s': %s", pushType, Arrays.stream(values) .map(v -> v instanceof Number ? v.toString() : String.format("'%s'", v)) From fb3b75b5608efc621cbefec31731a373a778a1e8 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 1 Jul 2024 16:57:22 -0700 Subject: [PATCH 32/33] Use suppliers for logging in MessageHandler --- .../main/java/glide/connectors/handlers/MessageHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java index 6cfce1f1e8..858af24d0f 100644 --- a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java @@ -75,7 +75,7 @@ public void handle(Response response) { Logger.log( Logger.Level.INFO, "subscribe/unsubscribe notification", - String.format( + () -> String.format( "Received push notification of type '%s': %s", pushType, Arrays.stream(values) @@ -86,7 +86,7 @@ public void handle(Response response) { Logger.log( Logger.Level.WARN, "unknown notification", - String.format("Unknown notification message: '%s'", pushType)); + () -> String.format("Unknown notification message: '%s'", pushType)); } } From d48f4fbe343aa07d20f575e2dbc55c09fa7ec971 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 1 Jul 2024 17:08:34 -0700 Subject: [PATCH 33/33] Address PR comments --- .../glide/connectors/handlers/MessageHandler.java | 13 +++++++------ .../connection/ConnectionWithGlideMockTests.java | 1 + java/src/lib.rs | 2 ++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java index 858af24d0f..da2d16502a 100644 --- a/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java +++ b/java/client/src/main/java/glide/connectors/handlers/MessageHandler.java @@ -75,12 +75,13 @@ public void handle(Response response) { Logger.log( Logger.Level.INFO, "subscribe/unsubscribe notification", - () -> String.format( - "Received push notification of type '%s': %s", - pushType, - Arrays.stream(values) - .map(v -> v instanceof Number ? v.toString() : String.format("'%s'", v)) - .collect(Collectors.joining(" ")))); + () -> + String.format( + "Received push notification of type '%s': %s", + pushType, + Arrays.stream(values) + .map(v -> v instanceof Number ? v.toString() : String.format("'%s'", v)) + .collect(Collectors.joining(" ")))); break; default: Logger.log( diff --git a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java index a945bdaaff..ebee688e69 100644 --- a/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java +++ b/java/client/src/test/java/glide/connection/ConnectionWithGlideMockTests.java @@ -38,6 +38,7 @@ public class ConnectionWithGlideMockTests extends RustCoreLibMockTestBase { @BeforeEach @SneakyThrows public void createTestClient() { + // TODO: Add DISABLED level to logger-core Logger.setLoggerConfig(Logger.Level.DISABLED); channelHandler = new ChannelHandler( diff --git a/java/src/lib.rs b/java/src/lib.rs index c8e809129d..d3adb935e8 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -320,6 +320,7 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local .unwrap_or(()) } +// TODO: Add DISABLED level here once it is added to logger-core impl From for Level { fn from(level: logger_core::Level) -> Self { match level { @@ -335,6 +336,7 @@ impl From for Level { impl TryFrom for logger_core::Level { type Error = FFIError; fn try_from(level: Level) -> Result>::Error> { + // TODO: Add DISABLED level here once it is added to logger-core match level.0 { 0 => Ok(logger_core::Level::Error), 1 => Ok(logger_core::Level::Warn),