Checking with logger.isDebugEnabled() it still a good practice for hot
paths to avoid the construction of intermediate object arrays to hold the
var-args. Some logging libraries have fixed argument optimizations for
var-args up to a specific limit. In very hot paths it is nice to
check logger.isDebugEnabled() once into a temp boolean value and then check
that in all the subsequent debug logging statements in the hot path.
-Jake
On Fri, Sep 7, 2018 at 11:18 AM Dan Smith <dsm...@pivotal.io> wrote:
> I think this pattern is a holdover from using log4j 1. In that version, you
> added an if check to avoid unnecessary string concatenation if debug was
> enabled.
>
>
> 1. if (logger.isDebugEnabled()) {
> 2. logger.debug("Logging in user " + user.getName() + " with
> birthday " + user.getBirthdayCalendar());
> 3. }
>
>
> Log4j2 lets you pass a format string, so you can just do this:
>
> logger.debug("Logging in user {} with birthday {}", user.getName(),
> user.getBirthdayCalendar());
>
>
> These examples come from the log4j2 docs:
>
> https://logging.apache.org/log4j/2.0/manual/api.html
>
> -Dan
>
> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan <gosulli...@apache.org>
> wrote:
>
> > Hi all,
> >
> > I've noticed a pattern in Geode where we wrap a log call in a check at
> the
> > same level, such as:
> >
> > if (logger.isDebugEnabled()) {
> > logger.debug("cleaning up incompletely started
> > DistributionManager due to exception", r);
> > }
> >
> > Is there any reason to do this? To my mind, it's an extra conditional
> that
> > should essentially be the first check inside `logger.debug(...)` anyways,
> > and it complicates the code and makes it less readable. I've even seen
> > places in the code which have `if (logger.isDebugEnabled())
> > logger.trace(...))` and such.
> >
> > I would like to propose that unless there is a compelling reason to use
> > this pattern, we remove all extra checks entirely.
> >
> > Galen
> >
>
