Skip to content

[#1201] improvement: only invoking LOG.debug when LOG.isDebugEnabled() is true - #1217

Merged
roryqi merged 2 commits into
apache:masterfrom
misselvexu:i/1201
Oct 8, 2023
Merged

[#1201] improvement: only invoking LOG.debug when LOG.isDebugEnabled() is true#1217
roryqi merged 2 commits into
apache:masterfrom
misselvexu:i/1201

Conversation

@misselvexu

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Improve invoking LOG.debug when LOG.isDebugEnabled() is true .

Why are the changes needed?

Fix: #1201

Does this PR introduce any user-facing change?

(Please list the user-facing changes introduced by your change, including

  1. Change in user-facing APIs.
  2. Addition or removal of property keys.)

No.

How was this patch tested?

(Please test your changes, and provide instructions on how to test it:

  1. If you add a feature or fix a bug, add a test to cover your changes.
  2. If you fix a flaky test, repeat it for many times to prove it works.)

@codecov-commenter

codecov-commenter commented Sep 26, 2023

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 2.75229% with 106 lines in your changes missing coverage. Please review.

Project coverage is 54.67%. Comparing base (b4adaa5) to head (eb5b7eb).
Report is 503 commits behind head on master.

Files with missing lines Patch % Lines
...pache/spark/shuffle/writer/WriteBufferManager.java 0.00% 7 Missing and 3 partials ⚠️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java 0.00% 9 Missing ⚠️
...java/org/apache/hadoop/mapred/SortWriteBuffer.java 0.00% 6 Missing and 1 partial ⚠️
...torage/handler/impl/HadoopShuffleWriteHandler.java 0.00% 5 Missing and 2 partials ⚠️
...he/uniffle/coordinator/CoordinatorGrpcService.java 0.00% 6 Missing ⚠️
...le/storage/handler/impl/LocalFileWriteHandler.java 0.00% 4 Missing and 2 partials ⚠️
...ain/java/org/apache/tez/common/TezClassLoader.java 0.00% 5 Missing ⚠️
...pache/uniffle/server/ShuffleServerGrpcService.java 0.00% 5 Missing ⚠️
...he/uniffle/server/buffer/ShuffleBufferManager.java 0.00% 5 Missing ⚠️
...java/org/apache/uniffle/client/RestClientImpl.java 0.00% 4 Missing ⚠️
... and 15 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1217      +/-   ##
============================================
+ Coverage     53.75%   54.67%   +0.92%     
- Complexity     2600     2611      +11     
============================================
  Files           392      372      -20     
  Lines         22486    20165    -2321     
  Branches       1887     1923      +36     
============================================
- Hits          12087    11026    -1061     
+ Misses         9689     8476    -1213     
+ Partials        710      663      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@advancedxy advancedxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And two meta questions:

  1. do we have a way to enforce this? such as style check.
  2. In java8, I believe we could use supplier to lazy logging message without the explicit check. Is it possible to upgrade the log4j version and leverages that. For example: https://stackoverflow.com/questions/47063232/lazy-evaluation-for-logging-in-java-8

@misselvexu

Copy link
Copy Markdown
Contributor Author

And two meta questions:

  1. do we have a way to enforce this? such as style check.
  2. In java8, I believe we could use supplier to lazy logging message without the explicit check. Is it possible to upgrade the log4j version and leverages that. For example: https://stackoverflow.com/questions/47063232/lazy-evaluation-for-logging-in-java-8

Currently the project is using log4j-1.2.17 version, theoretically it is possible to use log4j2 to replace, as for whether it can be fully compatible with other components still need to be tested.

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @misselvexu

@roryqi
roryqi merged commit 05aba70 into apache:master Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] only invoking LOG.debug when LOG.isDebugEnabled() is true

4 participants