[
https://issues.apache.org/jira/browse/ACCUMULO-3652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14508333#comment-14508333
]
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: https://issues.apache.org/jira/browse/ACCUMULO-3652
> 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
(v6.3.4#6332)
