-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Log4j 2 #20235
Introduce Log4j 2 #20235
Changes from 2 commits
5a8f2d7
7da0cde
abf8a1a
0853fc8
9a58fc2
abe3efd
1f6a4be
4e69ac0
0fdc5ca
1d197ed
21dbc5b
ac8c2e9
e166459
487ffe8
750033d
07d1a72
76ab02e
c98e790
d9064f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
|
||
package org.elasticsearch.action.admin.cluster.settings; | ||
|
||
import org.apache.logging.log4j.message.ParameterizedMessage; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.action.support.ActionFilters; | ||
|
@@ -148,7 +149,7 @@ public void onNoLongerMaster(String source) { | |
@Override | ||
public void onFailure(String source, Exception e) { | ||
//if the reroute fails we only log | ||
logger.debug("failed to perform [{}]", e, source); | ||
logger.debug(new ParameterizedMessage("failed to perform [{}]", source), e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this ParameterizedMessage something required for log4j2? It means we are constructing a new object on every logging call, regardless of whether the log message will be written (yes it is not as expensive as a string construction, but it is still doing allocation, and the whole point of parameterized log methods were to minimize work when no logging should be performed for the given level). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only needed for the invocations passing a throwable, because of the overrides that are available in the Log4j 2 API. That said, I used a script for converting all invocations of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed abf8a1a. |
||
listener.onFailure(new ElasticsearchException("reroute after update settings failed", e)); | ||
} | ||
|
||
|
@@ -166,7 +167,7 @@ public ClusterState execute(final ClusterState currentState) { | |
|
||
@Override | ||
public void onFailure(String source, Exception e) { | ||
logger.debug("failed to perform [{}]", e, source); | ||
logger.debug(new ParameterizedMessage("failed to perform [{}]", source), e); | ||
super.onFailure(source, e); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you at least add a comment with this linking to a followup issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #20243 and pushed 4e69ac0.