-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Improvement-13491][*] Use lombok @Slf4j annotation to generate logger #13509
Conversation
912775a
to
f5e5265
Compare
Codecov Report
@@ Coverage Diff @@
## dev #13509 +/- ##
============================================
- Coverage 39.62% 39.60% -0.02%
- Complexity 4357 4379 +22
============================================
Files 1097 1097
Lines 41161 41155 -6
Branches 4715 4714 -1
============================================
- Hits 16309 16299 -10
- Misses 23038 23046 +8
+ Partials 1814 1810 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
...src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java
Fixed
Show fixed
Hide fixed
lombok.config
Outdated
@@ -16,3 +16,5 @@ | |||
# | |||
|
|||
lombok.addLombokGeneratedAnnotation = true | |||
|
|||
lombok.log.fieldname=LOGGER |
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.
Why we don't keep the default field name log
?
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.
Lombok will generate a static final
field, so I changed the field name to uppercase. Is this inappropriate? Need to change to lowercase?
@Generated
private static final Logger LOGGER = LoggerFactory.getLogger(Xxx.class);
Why we don't keep the default field name
log
?
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 just think keep the default field name is better, since we already use @Slf4j
in some class, this will reduce the change.
lombok.config
Outdated
@@ -16,3 +16,5 @@ | |||
# | |||
|
|||
lombok.addLombokGeneratedAnnotation = true |
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 prefer that we keep the same formatting of this file. You might either remove the blanks around =
in this line or add blanks around =
in next line.
f5e5265
to
1ff64d5
Compare
1ff64d5
to
945b6c7
Compare
@@ -146,13 +146,13 @@ | |||
public boolean killTask() { | |||
|
|||
try { | |||
logger.info("Begin to kill task: {}", taskInstance.getName()); | |||
log.info("Begin to kill task: {}", taskInstance.getName()); |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null
c882da7
to
1319847
Compare
1319847
to
b0968fd
Compare
SonarCloud Quality Gate failed. |
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.
LGTM
Purpose of the pull request
close #13491
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md