Support source class & method in to-jul (LOG4J2-3282)#698
Support source class & method in to-jul (LOG4J2-3282)#698carterkozak merged 6 commits intoapache:release-2.xfrom
Conversation
Yessir! Looks great :-) I probably could have described it more clearly. Just needs a license header to appease rat. |
Duh! I should know better; done!
Done, now |
log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/LazyLog4jLogRecord.java
Outdated
Show resolved
Hide resolved
log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/LazyLog4jLogRecord.java
Outdated
Show resolved
Hide resolved
log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/LazyLog4jLogRecord.java
Outdated
Show resolved
Hide resolved
log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/LazyLog4jLogRecord.java
Show resolved
Hide resolved
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Please see my comments/questions. TY!
Done! |
| return super.getSourceMethodName(); | ||
| } | ||
|
|
||
| private synchronized void inferCaller() { |
There was a problem hiding this comment.
I don't think synchronized is necessary based on how this class is used and the thread-safety of the parent class. Synchronization here doesn't buy us anything because it only guards the method itself, not the inferCaller check elsewhere (which isn't volatile, nor do I think it should be) so it would only prevent additional concurrent calls from running in parallel.
There was a problem hiding this comment.
@garydgregory @carterkozak can I let you guys hash this out among ourselves and let me know your conclusion? I'm happy if you are (both) happy, but I'd prefer not going back and forth a few times between you... 😸 I believe this comment appears to request to (re-)remove synchronized (again, after @garydgregory suggested it be added), and also for volatile to be removed from inferCaller. Please both ACK?
There was a problem hiding this comment.
s/ourselves/yourselves/
There was a problem hiding this comment.
Let's remove the synchronized keyword please, sorry for the churn!
and also for volatile to be removed from inferCaller
I don't think inferCaller is volatile, no change needed there!
There was a problem hiding this comment.
OK, synchronized removed again now.
I don't think
inferCalleris volatile, no change needed there!
Duh, on TGIF the transient morphed into a volatile in my mind 😈
log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/LazyLog4jLogRecord.java
Outdated
Show resolved
Hide resolved
…rterkozak@ review
|
I approved the PR but as my comments are resolved. I'll let @carterkozak give the final nod.😉 |
|
Thanks, Gary :-) |
carterkozak
left a comment
carterkozak
left a comment
There was a problem hiding this comment.
Looks good, thanks! I'll get it merged once I have a moment :-)
3 participants
This is a LOG4J2-3282 related follow-up to #653.
@carterkozak is this what you meant in our conversation here? 😸 (Sorry at the time I didn't get it.)