remkop commented on pull request #208: URL: https://github.com/apache/logging-log4j2/pull/208#issuecomment-665536829
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 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 :sweat_smile: 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 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 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]
