You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by cakofony <gi...@git.apache.org> on 2018/02/14 19:49:38 UTC
[GitHub] logging-log4j2 pull request #150: Added ReusableMessage.forEachParameter
GitHub user cakofony opened a pull request:
https://github.com/apache/logging-log4j2/pull/150
Added ReusableMessage.forEachParameter
This method allows us to iterate over parameters in a
ReusableMessage without array creation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cakofony/logging-log4j2 message_parameter_for_each
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4j2/pull/150.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #150
----
commit 7fcacce40c6210d13200ced50dc517ccd612e5d7
Author: Carter Kozak <c4...@...>
Date: 2018-02-14T19:48:00Z
Added ReusableMessage.forEachParameter
This method allows us to iterate over parameters in a
ReusableMessage without array creation.
----
---
[GitHub] logging-log4j2 issue #150: LOG4J2-2253: Added ReusableMessage.forEachParamet...
Posted by remkop <gi...@git.apache.org>.
Github user remkop commented on the issue:
https://github.com/apache/logging-log4j2/pull/150
I haven't looked at #149 in detail yet, but wouldn't the ThreadLocal prevent the async logger background thread from seeing the parameter values?
---
[GitHub] logging-log4j2 issue #150: LOG4J2-2253: Added ReusableMessage.forEachParamet...
Posted by cakofony <gi...@git.apache.org>.
Github user cakofony commented on the issue:
https://github.com/apache/logging-log4j2/pull/150
The ThreadLocals would be constant sized arrays only used for calls to getParameters in order to avoid creating a new array.
Though now that I think through that idea it would have the potential for memory leaks unless the arrays are nulled out.
I think I've convinced myself that this PR is the way to go :-)
---
[GitHub] logging-log4j2 pull request #150: LOG4J2-2253: Added ReusableMessage.forEach...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/logging-log4j2/pull/150
---
[GitHub] logging-log4j2 issue #150: LOG4J2-2253: Added ReusableMessage.forEachParamet...
Posted by cakofony <gi...@git.apache.org>.
Github user cakofony commented on the issue:
https://github.com/apache/logging-log4j2/pull/150
Good call, updated.
Curious of your thoughts on this approach vs reusable threadlocal arrays to copy parameters into for Message.getParameters. That approach would have to copy a few references around, but the cost should be fairly small, and wouldn't necessarily work for messages with more than a few parameters, but those aren't particularly common anyhow. I suppose it's also more dangerous if consumers attempt to hold onto the returned parameter array across thread boundaries.
---
[GitHub] logging-log4j2 issue #150: LOG4J2-2253: Added ReusableMessage.forEachParamet...
Posted by cakofony <gi...@git.apache.org>.
Github user cakofony commented on the issue:
https://github.com/apache/logging-log4j2/pull/150
another option here is to update #149 to hold `ThreadLocal<Object[]>` for 1-3 parameters where we can copy chunks of the parameters array rather than allocating a new one.
That way we don't have to expand the API.
Thoughts?
---
[GitHub] logging-log4j2 issue #150: LOG4J2-2253: Added ReusableMessage.forEachParamet...
Posted by remkop <gi...@git.apache.org>.
Github user remkop commented on the issue:
https://github.com/apache/logging-log4j2/pull/150
I would like to add this method in a way that doesn’t break backwards compatibility. This can be done by putting the method in a new interface and let all `ReusableMessage` implementation classes also implement this new interface.
At the usage site, you’d check that the message is an instance of the new interface, and do a type cast so you can call the `forEach` method.
---