Improve LoggingCacheErrorHandler#28648
Conversation
At present, creating LoggingCacheErrorHandler that logs stacktrace also requires supplying the logger to be used. This might be inconvenient for some users, as it requires usage of Commons Logging API. This commit simplifies creation of such as LoggingCacheErrorHandler instance by adding a constructor that only accepts a boolean flag indicating whether to log stacktrace.
At present, LoggingCacheErrorHandler creates log message eagerly and does not verify whether warn level is enabled at all for the used logger. This commit ensures that log message creation is deferred until it is about to be logged.
e76f6f7 to
5da2dec
Compare
|
I think it's a good idea to check if the WARN log level is enabled, and switching to a The only real concern I have with the proposed changes is that the On the other hand, @snicoll, thoughts? |
|
Good point @sbrannen, I overlooked that I guess that for the --- a/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java
+++ b/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java
@@ -113,4 +113,15 @@ public class LoggingCacheErrorHandler implements CacheErrorHandler {
}
}
+ /**
+ * Log the specified message.
+ * @param logger the logger
+ * @param message the message
+ * @param ex the exception
+ * @deprecated as of 5.3.22 in favor of {@link #logCacheError(Log, Supplier, RuntimeException)}
+ */
+ @Deprecated
+ protected void logCacheError(Log logger, String message, RuntimeException ex) {
+ logCacheError(logger, () -> message, ex);
+ }
+
} |
Right. I also thought about doing that, but it would only cover half of the issue. Keeping the existing But it would not work for anyone who has overridden |
|
@vpavic, can you please submit the We can then get that merged in on its own since it's noncontroversial. And we can focus this PR on the discussion of the other proposed changes. |
|
Sure, I can do that: Though note there's one remark in the PR description that impacts the commit that's now subject of a separate PR:
|
Since LoggingCacheErrorHandler was only recently introduced in 5.3.16, we have decided to completely revise its internals (protected API) in 5.3.x while retaining the current public API. Specifically, this commit: - introduces protected getLogger() and isLogStackTraces() methods to improve extensibility - revises logCacheError() to accept a Supplier<String> for lazy resolution of error messages Closes spring-projectsgh-28672 See spring-projectsgh-28670, spring-projectsgh-28648
This PR contains 2 commits with some improvements to
LoggingCacheErrorHandler:Simplify creation of
LoggingCacheErrorHandlerwith logged stacktrace:At present, creating
LoggingCacheErrorHandlerthat logs stacktrace also requires supplying the logger to be used. This might be inconvenient for some users, as it requires usage of Commons Logging API. This commit simplifies creation of such asLoggingCacheErrorHandlerinstance by adding a constructor that only accepts a boolean flag indicating whether to log stacktrace.Defer log message creation in
LoggingCacheErrorHandler:At present,
LoggingCacheErrorHandlercreates log message eagerly and does not verify whether warn level is enabled at all for the used logger. This commit ensures that log message creation is deferred until it is about to be logged.The first commit could maybe be update to deprecate the existing constructor that takes
org.apache.commons.logging.Logand replace it with the one that takesStringrepresenting logger name as that way Commons Logging dependency wouldn't leak out at all. But I'd leave that decision to whoever reviews this PR.