At present, creating LoggingCacheErrorHandler with custom logger requires use of Commons Logging API, as the appropriate constructor expects org.apache.commons.logging.Log instance. As Commons Logging is rarely the logging framework of choice in applications these days, interaction with its API might not be desirable.

This commit adds LoggingCacheErrorHandler constructor that accepts logger name and thus avoids leaking out any details about the underlying logging framework, while also deprecating the existing constructor that accepts org.apache.commons.logging.Log.

I'm opening this PR to discuss one aspect of #28648 that was overlooked as that PR was superseded:

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.

These days, it's almost inevitable to have several logging frameworks on the classpath, with only one of those being the intended application-wide logging API. To avoid unintended use of other logging APIs, something like Checkstyle can be used to prohibit imports of undesirable classes. However, LoggingCacheErrorHandler makes this a bit difficult at the moment hence this proposal.

If you agree this proposal and also with the deprecation of constructor that uses org.apache.commons.logging.Log, I'll rework the tests to avoid use of deprecated constructor.

Comment From: vpavic

Could you consider this change for 6.0?

I'd like to be able to set up LoggingCacheErrorHandler with a custom logger, but without being forced to use org.apache.commons.logging.Log.

Thanks.

Comment From: jhoeller

Good point, we seem to have missed that part the first time around in the 5.3.22 revision.

I don't see a real need to deprecate the existing constructor (since we allow direct Log passing in other places as well) but we could easily add an alternative LoggingCacheErrorHandler(String loggerName, boolean logStackTraces) constructor for your scenario, avoiding Commons Logging API references in user code when specifying a custom logging category.

Drafting this locally, it is straightforward enough for a backport to 5.3.24, so I'm inclined to commit it there and repurpose this ticket for that addition to 5.3.x? Otherwise I can also create a separate GitHub issue for it.

Comment From: vpavic

Thanks for the feedback Juergen.

Not deprecating the constructor that accepts org.apache.commons.logging.Log is fine with me, I simply assumed once there's one that accepts the String, it would be preferred. Also great if you can make this a part of 5.3.

I can update the PR to remove the deprecation if that's your preference, and add some tests that use the new constructor.

Comment From: jhoeller

@vpavic If you could remove the deprecation and ideally rebase the PR onto 5.3.x (with a @since 5.3.24 label), I'd merge it there right away!

Comment From: vpavic

That is done now. I also added a simple test just for the sake of having something in the codebase that uses the newly introduced constructor.

Comment From: jhoeller

Thanks, that was quick :-)