Remove type casts in logging in server component#28807
Remove type casts in logging in server component#28807dnhatn merged 6 commits intoelastic:masterfrom
Conversation
We now can remove all type casts in logging as Eclipse can infer them correctly. This commit also removes incorrect usages of Supplier - we should use `java.util.Supplier` instead of `org.apache.logging.log4j.util`.
There was a problem hiding this comment.
What do you mean incorrect, there is not any method on Logger that accepts a java.util.Supplier, that actually can not be used here? Using a java.util.function.Supplier would actually be wrong because it would invoke
Logger#void info(Object message, Throwable t);
Please note that this change cause these method invocations to change from invoking:
Logger#void info(Supplier<?> msgSupplier, Throwable t);
to invoking
Logger#void info(MessageSupplier msgSupplier, Throwable t);
(for example). This is probably fine. Both of these will be removed in Log4j 3 where they will use JDK interfaces instead.
Historically, the reason that we use Supplier<?> is because MessageSupplier was initially deprecated in 2.6 (we started with Log4j 2.6.2). It has since been un-deprecated in 2.8.1 and is now deprecated again (for removal in Log4j 3). The Javadocs for Logger say this:
* Note that although {@link MessageSupplier} is provided, using {@link Supplier Supplier<Message>} works just the
* same. MessageSupplier was deprecated in 2.6 and un-deprecated in 2.8.1. Anonymous class usage of these APIs
* should prefer using Supplier instead.
Functionally, the two are equivalent because we supply Messages everywhere.
My preference is that we leave this as-is. Why? Because in Log4j 3 when these are removed we will break at compile time. That is a good thing, it means we can check that when we switch to using java.util.function.Supplier we are invoking the desired method (rather than this silently switching on us from Logger#void info(MessageSupplier msgSupplier, Throwable t); to something we do not check. The explicitness is advantageous here.
|
@jasontedor Thank you for your clear explanation.
I meant we should not use Actually @ywelsch suggested me to clean up these (#28534 (comment)) |
|
Well, I think what we have here is confusion that arises when two distinct changes are attempted in the same pull request. Changing the usage in |
|
@jasontedor I opened #28812 to replace log4j.Supplier by jdk’s in non-logging usage.
I am fine to close this. I don't have any objection here. |
# Conflicts: # server/src/main/java/org/elasticsearch/action/support/replication/TransportWriteAction.java # server/src/main/java/org/elasticsearch/client/transport/TransportClientNodesService.java # server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataMappingService.java # server/src/main/java/org/elasticsearch/cluster/service/ClusterApplierService.java # server/src/main/java/org/elasticsearch/common/util/IndexFolderUpgrader.java # server/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java # server/src/main/java/org/elasticsearch/tasks/TaskResultsService.java # server/src/main/java/org/elasticsearch/transport/TcpTransport.java
|
As discussed with Yannick, this PR can be proceeded (after #28969). |
|
@jasontedor Thanks for reviewing! |
This commit removes type-casts in logging in the server component (other components will be done later). This also adds a parameterized message test which would catch breaking-changes related to lambdas in Log4J.
* es/master: (27 commits) [Docs] Add rank_eval size parameter k (#29218) [DOCS] Remove ignore_z_value parameter link Docs: Update docs/index_.asciidoc (#29172) Docs: Link C++ client lib elasticlient (#28949) [DOCS] Unregister repository instead of deleting it (#29206) Docs: HighLevelRestClient#multiSearch (#29144) Add Z value support to geo_shape Remove type casts in logging in server component (#28807) Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878) REST : Split `RestUpgradeAction` into two actions (#29124) Add error file docs to important settings Add note to low-level client docs for DNS caching (#29213) Harden periodically check to avoid endless flush loop (#29125) Remove deprecated options for query_string (#29203) REST high-level client: add force merge API (#28896) Remove license information from README.textile (#29198) Decouple more classes from XContentBuilder and make builder strict (#29197) [Docs] Fix missing closing block in cluster/misc.asciidoc RankEvalRequest should implement IndicesRequest (#29188) Use EnumMap in ClusterBlocks (#29112) ...
* es/6.x: (29 commits) [Docs] Add rank_eval size parameter k (#29218) Docs: Update docs/index_.asciidoc (#29172) Docs: Link C++ client lib elasticlient (#28949) Docs: HighLevelRestClient#multiSearch (#29144) [DOCS] Remove ignore_z_value parameter link Add Z value support to geo_shape Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878) REST : Split `RestUpgradeAction` into two actions (#29124) [DOCS] Unregister repository instead of deleting it (#29206) Remove type casts in logging in server component (#28807) Add error file docs to important settings Add note to low-level client docs for DNS caching (#29213) testShrinkAfterUpgrade should only set mapping.single_type if bwc version > 5.5.0 Harden periodically check to avoid endless flush loop (#29125) REST high-level client: add force merge API (#28896) Remove license information from README.textile (#29198) Decouple more classes from XContentBuilder and make builder strict (#29197) Propagate mapping.single_type setting on shrinked index (#29202) [Docs] Fix missing closing block in cluster/misc.asciidoc RankEvalRequest should implement IndicesRequest (#29188) ...
* master: (40 commits) Do not optimize append-only if seen normal op with higher seqno (elastic#28787) [test] packaging: gradle tasks for groovy tests (elastic#29046) Prune only gc deletes below local checkpoint (elastic#28790) remove testUnassignedShardAndEmptyNodesInRoutingTable elastic#28745: remove extra option in the composite rest tests Fold EngineDiskUtils into Store, for better lock semantics (elastic#29156) Add file permissions checks to precommit task Remove execute mode bit from source files Optimize the composite aggregation for match_all and range queries (elastic#28745) [Docs] Add rank_eval size parameter k (elastic#29218) [DOCS] Remove ignore_z_value parameter link Docs: Update docs/index_.asciidoc (elastic#29172) Docs: Link C++ client lib elasticlient (elastic#28949) [DOCS] Unregister repository instead of deleting it (elastic#29206) Docs: HighLevelRestClient#multiSearch (elastic#29144) Add Z value support to geo_shape Remove type casts in logging in server component (elastic#28807) Change BroadcastResponse from ToXContentFragment to ToXContentObject (elastic#28878) REST : Split `RestUpgradeAction` into two actions (elastic#29124) Add error file docs to important settings ...
This commit removes type-casts in logging in the server component (other components will be done later). This also adds a parameterized message test which would be able to catch breaking-changes related to lambdas in Log4J.