Skip to content

Fix wrong logger names#3075

Merged
wu-sheng merged 3 commits intoapache:masterfrom
kezhenxu94:bugfix/logger-name
Jul 14, 2019
Merged

Fix wrong logger names#3075
wu-sheng merged 3 commits intoapache:masterfrom
kezhenxu94:bugfix/logger-name

Conversation

@kezhenxu94
Copy link
Copy Markdown
Member

Please answer these questions before submitting pull request

  • Why submit this pull request?
  • Bug fix
  • New feature provided
  • Improve performance

Logger name differentiate from the real class may be troublesome when debugging, and we have several pull requests that try to correct some of them (#3054), but not all of them.

So I just wrote a simple shell script to find out all such cases and fix them once and for all.

Since we are using lombok, to avoid such problem in the future, I'd recommend using @Slf4j annotation in future PRs when possible.

@kezhenxu94 kezhenxu94 added the bug Something isn't working and you are sure it's a bug! label Jul 14, 2019
@kezhenxu94 kezhenxu94 added this to the 6.3.0 milestone Jul 14, 2019
@kezhenxu94 kezhenxu94 changed the title Fix wrong logger name Fix wrong logger names Jul 14, 2019
wu-sheng
wu-sheng previously approved these changes Jul 14, 2019
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM

Since we are using lombok, to avoid such problem in the future, I'd recommend using @slf4j annotation in future PRs when possible.

This is not working at the agent side. How about adding your script in CI?

@wu-sheng
Copy link
Copy Markdown
Member

Since we are using lombok, to avoid such problem in the future, I'd recommend using @slf4j annotation in future PRs when possible.

@kezhenxu94 You could change this in OAP side, I am just saying, the check may be still necessary.

@wu-sheng
Copy link
Copy Markdown
Member

Are we going to merge this first?

@kezhenxu94
Copy link
Copy Markdown
Member Author

kezhenxu94 commented Jul 14, 2019

Are we going to merge this first?

I've just fixed those in agent side, and can be merged first now.

As for the check in CI, let me update the script and work on it in another PR

Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM

@kezhenxu94
Copy link
Copy Markdown
Member Author

/run e2e

@wu-sheng wu-sheng merged commit 4db1773 into apache:master Jul 14, 2019
@kezhenxu94 kezhenxu94 deleted the bugfix/logger-name branch July 14, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working and you are sure it's a bug!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants