Skip to content

*: enable bodyclose linter #4437

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
Merged
merged 2 commits into from
Dec 8, 2021
Merged

*: enable bodyclose linter #4437

merged 2 commits into from
Dec 8, 2021

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Dec 7, 2021 •
edited
Loading

Signed-off-by: Ryan Leung rleungx@gmail.com

What problem does this PR solve?

ref #4322

What is changed and how it works?

Enable bodyclose linter.

Check List

Tests

  • Unit test

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 7, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • JmPotato
  • lhy1024

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/invalid-commit-message release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 7, 2021
@rleungx rleungx changed the title enable bodyclose linter *: enable bodyclose linter Dec 7, 2021
@rleungx
enable bodyclose linter
158ebbf 
ref tikv#4322

Signed-off-by: Ryan Leung <rleungx@gmail.com>
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/invalid-commit-message release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 7, 2021
@rleungx rleungx added component/misc Some other things. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 7, 2021
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 7, 2021
@rleungx rleungx marked this pull request as ready for review December 7, 2021 08:17
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 7, 2021 •
edited
Loading

Codecov Report

Merging #4437 (0819a58) into master (90a9689) will increase coverage by 0.18%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4437      +/-   ##
==========================================
+ Coverage   74.71%   74.90%   +0.18%     
==========================================
  Files         263      263              
  Lines       27629    27678      +49     
==========================================
+ Hits        20642    20731      +89     
+ Misses       5130     5109      -21     
+ Partials     1857     1838      -19
Flag Coverage Δ
unittests 74.90% <83.33%> (+0.18%) ⬆️

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

Impacted Files Coverage Δ
server/statistics/kind.go 17.24% <0.00%> (-2.76%) ⬇️
server/api/scheduler.go 47.08% <66.66%> (ø)
server/api/util.go 53.01% <77.77%> (ø)
pkg/movingaverage/avg_over_time.go 100.00% <100.00%> (ø)
pkg/movingaverage/median_filter.go 100.00% <100.00%> (ø)
pkg/movingaverage/queue.go 100.00% <100.00%> (ø)
pkg/movingaverage/time_median.go 100.00% <100.00%> (ø)
server/cluster/cluster.go 82.56% <100.00%> (-0.81%) ⬇️
server/server.go 72.45% <100.00%> (ø)
server/statistics/hot_peer.go 100.00% <100.00%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2246ef6...0819a58. Read the comment docs.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 7, 2021
disksing
disksing reviewed Dec 7, 2021
View reviewed changes
server/api/util.go Outdated
res, err := client.Do(req)
if err != nil {
return nil, err
return http.StatusInternalServerError, err
Copy link
Contributor

Choose a reason for hiding this comment

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

can we return 0, err here?

Copy link
Member Author

@rleungx rleungx Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The caller will get 0 as the status code which is not good. I think an alternative is to return the status code according to the detailed error. But I'm going to leave it for now since it could happen rarely.

Copy link
Contributor

Choose a reason for hiding this comment

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

0 is ok to use as an invalid status code when err is not nil. If you construct status code in client side, we cannot distiguish the cases that we receive the status code from server or construct it from client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense, change to 0.

JmPotato
JmPotato reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@JmPotato JmPotato left a comment

Choose a reason for hiding this comment

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

The rest LGTM.

tests/server/api/api_test.go
@@ -70,6 +70,7 @@ func (s *serverTestSuite) TestReconnect(c *C) {
if name != leader {
res, e := http.Get(s.GetConfig().AdvertiseClientUrls + "/pd/api/v1/version")
c.Assert(e, IsNil)
res.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Why not defer?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a for loop, closing it directly is better.

@rleungx
address the comment
0819a58 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
Copy link
Member Author

rleungx commented Dec 8, 2021

@JmPotato @disksing PTAL

@rleungx
Copy link
Member Author

rleungx commented Dec 8, 2021

/run-unit-tests

@rleungx rleungx requested review from JmPotato and disksing December 8, 2021 05:02
@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 Dec 8, 2021
@rleungx
Copy link
Member Author

rleungx commented Dec 8, 2021

/merge

@ti-chi-bot
Copy link
Member

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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.

@ti-chi-bot
Copy link
Member

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

Commit hash: 0819a58

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 8, 2021
@ti-chi-bot ti-chi-bot merged commit 4ec9e14 into tikv:master Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JmPotato JmPotato JmPotato approved these changes

@lhy1024 lhy1024 lhy1024 approved these changes

@nolouch nolouch Awaiting requested review from nolouch

@disksing disksing Awaiting requested review from disksing

Assignees
No one assigned
Labels
component/misc Some other things. release-note-none Denotes a PR that doesn't merit a release note. 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.
5 participants
@rleungx @ti-chi-bot @JmPotato @disksing @lhy1024