This PR contains 2 commits with some improvements to LoggingCacheErrorHandler:

1. Simplify creation of LoggingCacheErrorHandler with logged stacktrace: 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.

2. Defer log message creation in LoggingCacheErrorHandler: 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.

The first commit could maybe be update to deprecate the existing constructor that takes org.apache.commons.logging.Log and replace it with the one that takes String representing 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.

Comment From: sbrannen

I think it's a good idea to check if the WARN log level is enabled, and switching to a Supplier is also a good improvement.

The only real concern I have with the proposed changes is that the logCacheError() method is protected. Thus, that is technically a breaking change in case somebody has subclassed LoggingCacheErrorHandler.

On the other hand, LoggingCacheErrorHandler was introduced in 5.3.16, so perhaps there is little risk in changing the signature of the protected method at this point.

@snicoll, thoughts?

Comment From: vpavic

Good point @sbrannen, I overlooked that #logCacheError is protected.

I guess that for the 5.3.x branch it shouldn't be an issue to ensure backwards compatibility using something like this:

--- 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);
+       }
+
 }

Comment From: sbrannen

I guess that for the 5.3.x branch it shouldn't be an issue to ensure backwards compatibility using something like this:

Right. I also thought about doing that, but it would only cover half of the issue.

Keeping the existing logCacheError() method would work for people who have extended LoggingCacheErrorHandler, overridden one of the handle*() methods, and invoked logCacheError().

But it would not work for anyone who has overridden logCacheError(Log, String, RuntimeException) since the new code (in this PR) never invokes logCacheError(Log, String, RuntimeException). In other words, any existing customization in an overridden logCacheError(Log, String, RuntimeException) method would be ignored.

Comment From: sbrannen

@vpavic, can you please submit the Simplify creation of LoggingCacheErrorHandler with logged stacktrace commit in a separate PR?

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.

Comment From: vpavic

Sure, I can do that:

• 28670

Though note there's one remark in the PR description that impacts the commit that's now subject of a separate PR:

The first commit could maybe be update to deprecate the existing constructor that takes org.apache.commons.logging.Log and replace it with the one that takes String representing 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.

Comment From: sbrannen

Thanks for the fruitful discussions, @vpavic!

This PR has now been superseded by #28670 and #28672.