*: enable errcheck #8277

okJiang · 2024-06-12T04:06:23Z

What problem does this PR solve?

Issue Number: Ref #1919

What is changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: okJiang <[email protected]>

codecov · 2024-06-12T04:17:20Z

Codecov Report

Attention: Patch coverage is 47.22222% with 19 lines in your changes missing coverage. Please review.

Project coverage is 77.28%. Comparing base (9dff6e6) to head (b2b5ab6).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8277      +/-   ##
==========================================
- Coverage   77.35%   77.28%   -0.08%     
==========================================
  Files         470      470              
  Lines       61376    61385       +9     
==========================================
- Hits        47478    47439      -39     
- Misses      10336    10388      +52     
+ Partials     3562     3558       -4

Flag	Coverage Δ
unittests	`77.28% <47.22%> (-0.08%)`	⬇️

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

client/pd_service_discovery.go

client/resource_manager_client.go

pkg/mcs/scheduling/server/cluster.go

pkg/mcs/utils/util.go

server/cluster/cluster.go

pkg/replication/replication_mode.go

HuSharp · 2024-06-12T04:44:11Z

.golangci.yml

+    - path: (pd-analysis|pd-api-bench|pd-backup|pd-ctl|pd-heartbeat-bench|pd-recover|pd-simulator|pd-tso-bench|pd-ut|regions-dump|stores-dump)
+      linters:
+        - errcheck
+    - path: (pkg/schedule/labeler/labeler.go|pkg/mcs/tso/server/config.go|pkg/tso/admin.go|pkg/mcs/tso/server/grpc_service.go|pkg/schedule/schedulers/split_bucket.go|server/api/plugin_disable.go|server/api/plugin_disable.go|server/api/operator.go|server/api/region.go|pkg/schedule/schedulers/balance_leader.go|pkg/mcs/resourcemanager/server/server.go|pkg/mcs/scheduling/server/grpc_service.go|pkg/mcs/resourcemanager/server/.*\.go|plugin/scheduler_example/evict_leader.go|server/api/.*\.go|pkg/replication/replication_mode.go|pkg/mcs/scheduling/server/server.go|pkg/storage/endpoint/gc_safe_point.go|server/.*\.go|pkg/schedule/schedulers/.*\.go|pkg/schedule/placement/rule.go|pkg/mcs/utils/util.go|pkg/keyspace/tso_keyspace_group.go|pkg/tso/allocator_manager.go|pkg/core/store_stats.go|pkg/autoscaling/handler.go|pkg/core/store_stats.go|pkg/keyspace/keyspace.go|pkg/storage/hot_region_storage.go|pkg/syncer/server.go)


Where did these paths come from, handwritten? 0 0
If a new folder is created, do I need to add it here manually?

Where did these paths come from, handwritten? 0 0

yes, these files lack too much errcheck. I can't fix them in this one pr, so ignore them for now

If a new folder is created, do I need to add it here manually?

no, you need to fix the errcheck hint of your new file

HuSharp · 2024-06-12T04:44:53Z

client/tso_client.go

@@ -118,7 +118,9 @@ func (c *tsoClient) getOption() *option { return c.option }
 func (c *tsoClient) getServiceDiscovery() ServiceDiscovery { return c.svcDiscovery }

 func (c *tsoClient) setup() {
-	c.svcDiscovery.CheckMemberChanged()
+	if err := c.svcDiscovery.CheckMemberChanged(); err != nil {
+		log.Warn("[tso] failed to check member changed", errs.ZapError(err))


using log.warn or log.error?

I'm not very sure either, because this error was ignored before, so I used "warn"

It's ok to use warn because CheckMemberChanged will also output some error logs internally.

Signed-off-by: okJiang <[email protected]>

rleungx

The rest LGTM

pkg/replication/replication_mode_test.go

Signed-off-by: okJiang <[email protected]>

…rrcheck

okJiang · 2024-06-13T08:41:59Z

/retest

JmPotato · 2024-06-14T06:30:38Z

/merge

ti-chi-bot · 2024-06-14T06:30:40Z

@JmPotato: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: Proposal: Strengthen configuration change approval.

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.

JmPotato · 2024-06-14T06:31:03Z

/approve

ti-chi-bot · 2024-06-14T06:33:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, rleungx

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JmPotato,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-06-14T06:33:09Z

[LGTM Timeline notifier]

Timeline:

2024-06-13 08:11:51.827623148 +0000 UTC m=+625065.880935095: ☑️ agreed by rleungx.
2024-06-14 06:33:08.722633628 +0000 UTC m=+705542.775945553: ☑️ agreed by JmPotato.

rleungx · 2024-06-14T06:34:10Z

/merge

ti-chi-bot · 2024-06-14T06:34:11Z

@rleungx: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: Proposal: Strengthen configuration change approval.

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.

enable errcheck

25ca220

Signed-off-by: okJiang <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2024

add comment

52c1e83

Signed-off-by: okJiang <[email protected]>

rleungx reviewed Jun 12, 2024

View reviewed changes

HuSharp reviewed Jun 12, 2024

View reviewed changes

okJiang added 3 commits June 12, 2024 13:09

fix comment

adb5543

Signed-off-by: okJiang <[email protected]>

fix comment: remove log in hot path

2dcafe6

Signed-off-by: okJiang <[email protected]>

Merge branch 'master' of github.com:tikv/pd into enable-errcheck

1e781f3

Signed-off-by: okJiang <[email protected]>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2024

Merge branch 'master' into enable-errcheck

711c6cb

rleungx approved these changes Jun 13, 2024

View reviewed changes

pkg/replication/replication_mode_test.go Outdated Show resolved Hide resolved

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 13, 2024

okJiang added 2 commits June 13, 2024 16:29

fix comment

88dc908

Signed-off-by: okJiang <[email protected]>

Merge branch 'enable-errcheck' of github.com:okJiang/pd into enable-e…

81da630

…rrcheck

Merge branch 'master' into enable-errcheck

f62b6c6

okJiang mentioned this pull request Jun 14, 2024

let errcheck pass in ci #1919

Closed

JmPotato approved these changes Jun 14, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 14, 2024

ti-chi-bot bot added 2 commits June 14, 2024 06:34

Merge branch 'master' into enable-errcheck

740b2f4

Merge branch 'master' into enable-errcheck

b2b5ab6

ti-chi-bot bot merged commit 13174b5 into tikv:master Jun 14, 2024
18 of 19 checks passed

okJiang deleted the enable-errcheck branch June 14, 2024 08:07

okJiang mentioned this pull request Jun 21, 2024

*: enable errcheck for server/*.go except server/api #8317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: enable errcheck #8277

*: enable errcheck #8277

okJiang commented Jun 12, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

HuSharp Jun 12, 2024

okJiang Jun 12, 2024 •

edited

Loading

HuSharp Jun 12, 2024

okJiang Jun 12, 2024

JmPotato Jun 13, 2024

rleungx left a comment

okJiang commented Jun 13, 2024

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

rleungx commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

*: enable errcheck #8277

*: enable errcheck #8277

Conversation

okJiang commented Jun 12, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

HuSharp Jun 12, 2024

Choose a reason for hiding this comment

okJiang Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

HuSharp Jun 12, 2024

Choose a reason for hiding this comment

okJiang Jun 12, 2024

Choose a reason for hiding this comment

JmPotato Jun 13, 2024

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

okJiang commented Jun 13, 2024

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

JmPotato commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

[LGTM Timeline notifier]

rleungx commented Jun 14, 2024

ti-chi-bot bot commented Jun 14, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

okJiang Jun 12, 2024 •

edited

Loading