Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cdc,pkg,tests(ticdc): unify cdc log style #4235

Merged
merged 6 commits into from
Jan 6, 2022
Merged

cdc,pkg,tests(ticdc): unify cdc log style #4235

merged 6 commits into from
Jan 6, 2022

Conversation

overvenus
Copy link
Member

@overvenus overvenus commented Jan 6, 2022 •
edited
Loading

What problem does this PR solve?

  • Prefer using "duration" for slow logs
  • Prefer using camelCase for zap field names

Issue Number: close #4239

Check List

Tests

  • Unit test
  • Integration test

Release note 

None

@overvenus
cdc,pkg,tests(ticdc): unify cdc log style
ac1120d 
* Prefer using "duration" for slow logs
* Prefer using camelCase for zap field names

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus added component/metrics-logging Metrics and logging component. area/ticdc Issues or PRs related to TiCDC. labels Jan 6, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 6, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • amyangfei
  • asddongmen

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2022
@overvenus
Makefile(ticdc): add log style check script
3b19ade 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022 •
edited
Loading

Codecov Report

Merging #4235 (fe3429c) into master (08da001) will decrease coverage by 0.0050%.
The diff coverage is 52.4075%.

Flag Coverage Δ
cdc 58.5649% <57.4314%> (-0.0799%) ⬇️
dm 52.2527% <40.5451%> (-0.0018%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #4235        +/-   ##
================================================
- Coverage   55.1722%   55.1672%   -0.0051%     
================================================
  Files           485        491         +6     
  Lines         59829      60942      +1113     
================================================
+ Hits          33009      33620       +611     
- Misses        23484      23937       +453     
- Partials       3336       3385        +49

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2022
amyangfei
amyangfei reviewed Jan 6, 2022
View reviewed changes
scripts/check-log-style.sh Outdated
# limitations under the License.

# zap field name should be camelCase, excepts for idioms and special terms.
grep -RnE "zap.[A-Z][a-z]+\(\".*-.*\"" cdc tests pkg |
Copy link
Contributor

Choose a reason for hiding this comment

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

zap.String("node name", r.name)
This zap filed style (words split by space) is not checked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed, thanks!

@overvenus
cdc,pkg(ticdc): do not use space and underscore
fe3429c 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 6, 2022
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 6, 2022
@asddongmen
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: fe3429c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 6, 2022
@ti-chi-bot
Copy link
Member

@overvenus: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@overvenus
Copy link
Member Author

/run-verify
/run-integration-test
/run-dm-integration-test

@ti-chi-bot ti-chi-bot merged commit 4f47b9e into pingcap:master Jan 6, 2022
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jan 18, 2022
@overvenus
cdc,pkg,tests(ticdc): unify cdc log style (pingcap#4235)
18cfe11 
close pingcap#4239
@overvenus overvenus mentioned this pull request Jan 18, 2022
Closed
zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Jan 20, 2022
@overvenus @zhaoxinyu
cdc,pkg,tests(ticdc): unify cdc log style (pingcap#4235)
a2194fb 
close pingcap#4239
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 21, 2022
@overvenus
cdc,pkg,tests(ticdc): unify cdc log style (pingcap#4235)
e8cedf8 
close pingcap#4239

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus mentioned this pull request Jun 21, 2022
Merged
overvenus added a commit to ti-chi-bot/tiflow that referenced this pull request Jun 23, 2022
@overvenus
cdc,pkg,tests(ticdc): unify cdc log style (pingcap#4235)
c7b7732 
close pingcap#4239

Signed-off-by: Neil Shen <overvenus@gmail.com>
ti-chi-bot pushed a commit that referenced this pull request Jun 23, 2022
@overvenus @ti-chi-bot
cdc,pkg,tests(ticdc): unify cdc log style (#4235)
cd49db6 
close #4239

Signed-off-by: Neil Shen <overvenus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei approved these changes

@asddongmen asddongmen asddongmen approved these changes

Assignees
No one assigned
Labels
area/ticdc Issues or PRs related to TiCDC. component/metrics-logging Metrics and logging component. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify log style and zap field names
5 participants
@overvenus @ti-chi-bot @codecov-commenter @asddongmen @amyangfei