accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ed Coleman (JIRA)" <>
Subject [jira] [Commented] (ACCUMULO-3652) Remove string concatenation in log statements where slf4j is used.
Date Thu, 23 Apr 2015 02:33:39 GMT


Ed Coleman commented on ACCUMULO-3652:

Bob, it looks like the pull request has been closed. I have not used the review board (yet)
so I can't be much help there, but I would like to look at the changes - but not sure what
version you have is your latest.

When I made the previous changes, I would commit to my local branch often, but then I squashed
those merges into one commit and I then fast-forwarded my changes to the latest accumulo-master
(re-squishing as necessary) and then make a patch against my branch and then submitted that,
 The patch submission took two steps in Jira - one to say patch available and a second to
actually attach the file.

>From what I see from following the conversation it seems that you are real close and I'd
like to help to close this out.

One thing that I want to see in the review is the changes that you have made in OpTimer and
also those dealing with TLevel commented on above.

This is my personnel opinion, but I've taken a couple of runs at the logging issue.  The ultimate
goal it to enable Accumulo to move forward with a more modern logging framework (log4j2, logback,...)
and to allow clients the flexibility to chose what they want. 

At this point, Accumulo (as well as some dependencies, i.e.hadoop) are pretty entrenched with
log4j and we cannot remove it from the back end until there is some major refactoring / changes
to some fundamental Accumulo components. (There is still hope for the client though.)

OpTimer is one, and the best approach may be to substitute other functionality that is compatible.
 There is a separate ticket that expresses this desire.

TLevel and forwarding messages to the monitor is another tough hill to climb. This will really
require a lot of rework in a very core component - something that I think we need to agree
on an approach and it will probably need to be part of a major release.

With these two and other similar issues I've been taking the approach that it really should
be all or nothing. There is no problem continuing with log4j for the time being and making
changes, even if they seem minor could turn out to have real issues.  And being that these
are fundamental core components / functionality, well the risk seems to outweigh any benefits
of a partial solution - if the resulting component is still going to have a direct log4j dependency
then I am in favor of leaving it until a solution is decided on. Otherwise, I think I'd need
to see some specific tests added to demonstrate that there really is no change in functionality.

As a hypothetical - say that a change prevented some messages from being forwarded to the
monitor. We may never know until someone had an error in a tserver log that should have been
displayed in the monitor but wasn't. Chasing the issue down would be quite tedious and might
even be over looked for quite awhile - blamed on other things,... So, with little benefit
now, and the component still having a direct log4j dependency (even if reduced) does not seem
worth it. Completely refactoring and adequately testing a new component seems a more conservative
approach that will minimize surprises.

These first couple of passes have allowed us to see where we still have issues and your work
will help this move forward. Thanks.

Let me know what I can do to help.

> Remove string concatenation in log statements where slf4j is used.
> ------------------------------------------------------------------
>                 Key: ACCUMULO-3652
>                 URL:
>             Project: Accumulo
>          Issue Type: Sub-task
>          Components: build
>    Affects Versions: 1.7.0
>            Reporter: Ed Coleman
>            Assignee: Bob Thorman
>            Priority: Minor
>             Fix For: 1.7.0
>         Attachments: ACCUMULO-3652-3.patch
> As a separate task, continue with the conversion to remove log4j dependencies, modify
log statements where string concatenation is used and replace with {} parameter substitution.

This message was sent by Atlassian JIRA

View raw message