garydgregory commented on pull request #208: URL: https://github.com/apache/logging-log4j2/pull/208#issuecomment-665636379
On Wed, Jul 29, 2020 at 5:02 AM Remko Popma <[email protected]> wrote: > I did not have time to do an in-depth review today, would need to take > another detailed look. > > It has been a while since I looked, perhaps @bjlaub > <https://github.com/bjlaub> can clarify whether my understanding of the > points below is correct? > > - The goal of this PR is to improve performance for scenarios where > garbage-free logging is combined with Async Loggers. This is achieved by > doing some formatting work, which is usually done in the application > thread, in the background thread instead. It should *only* impact > cases where garbage-free logging is combined with Async Loggers, is that > correct? - Note that I need to take a more detailed look to see how the PR > actually works. > - This logic only activates if Constants.FORMAT_MESSAGES_IN_BACKGROUND > (system property log4j.format.msg.async) is true. Is that correct? > > Overall: > > Note that using this FORMAT_MESSAGES_IN_BACKGROUND configuration is a > fairly risky thing to do for applications: the application developers need > to be *very* sure that *none* of the objects given as parameters to > logging calls are mutated after the logging call, or the log may contain a > different value from what the actual value was when the logging call was > made. If someone who is not aware of this configuration is troubleshooting > some problem, then this can be highly confusing. I admit I am responsible > for this 😅 since I implemented an early version of this configuration > but I am no longer sure that this was a good idea... > > Another, related, consideration is that this area (async loggers and > garbage-free logging) is complex and a bit fragile. Again, this is my > responsibility and not criticism of @bjlaub <https://github.com/bjlaub> > or anyone else. However, because of this, I believe it is easy to introduce > bugs, and so perhaps the bar for accepting changes in this area needs to be > a bit higher than usual. I am not yet sure that even a 10x throughput > improvement in the formatting is a good trade-off when this advantage can > only be achieved by the rare applications that are willing to switch on > this (risky) FORMAT_MESSAGES_IN_BACKGROUND configuration. > This feature seems like a land mine waiting for activation in any non-trivial software stack, especially as activated by a global setting any code and toggle. I would be OK with it being a feature I enabled in a LoggerContext or better yet in a MessageFactory. Gary > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/logging-log4j2/pull/208#issuecomment-665536829>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAJB6N6NUY6EZN4E3TQMVKDR57QTNANCNFSM4FOXQAQA> > . > ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
