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

okJiang · 2024-06-21T08:29:19Z

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 <819421878@qq.com>

cmd/pd-server/main.go

rleungx · 2024-06-21T08:46:31Z

pkg/replication/replication_mode.go

@@ -232,46 +232,44 @@ func (m *ModeManager) loadDRAutoSync() error {
 	return nil
 }

-func (m *ModeManager) drSwitchToAsyncWait(availableStores []uint64) error {
+func (m *ModeManager) drSwitchToAsyncWait(availableStores []uint64) {


Why change it?

The caller always ignored this error, and all errors were logged within the function, so I removed it.

But the tests will check the return value of drSwitchToAsync.

Got it. I will revert it for the test

rleungx · 2024-06-21T08:46:38Z

pkg/replication/replication_mode.go

 }

-func (m *ModeManager) drSwitchToAsync(availableStores []uint64) error {
+func (m *ModeManager) drSwitchToAsync(availableStores []uint64) {


Signed-off-by: okJiang <819421878@qq.com>

okJiang · 2024-06-24T03:32:10Z

/cc @JmPotato @HuSharp

rleungx · 2024-06-24T04:06:19Z

pkg/mcs/registry/registry.go

-			l.RegisterRESTHandler(h)
-			log.Info("restful API service already registered", zap.String("prefix", prefix), zap.String("service-name", name))
+			if err := l.RegisterRESTHandler(h); err != nil {
+				log.Error("register REST handler failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))


Suggested change

log.Error("register REST handler failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))

log.Error("register restful API service failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))

rleungx · 2024-06-24T04:09:09Z

pkg/storage/endpoint/gc_safe_point.go

@@ -100,7 +100,9 @@ func (se *StorageEndpoint) LoadMinServiceGCSafePoint(now time.Time) (*ServiceSaf
 		}

 		if ssp.ExpiredAt < now.Unix() {
-			se.Remove(key)
+			if err := se.Remove(key); err != nil {


Do we need to return the error here?

fixed in c01b053

rleungx · 2024-06-24T04:10:50Z

pkg/syncer/server.go

-				s.limit.WaitN(ctx, resp.Size())
+				if err := s.limit.WaitN(ctx, resp.Size()); err != nil {
+					log.Error("failed to wait rate limit", errs.ZapError(err))
+					return err


Do we need to return?

I think it's possible to return an error here.

After checking the source code, there are two types of errors that WaitN returns, fmt.Errorf("rate: Wait(n=%d) exceeds limiter's burst %d", n, burst) and context errors.

For the former. If resp.Size() exceeds the burst, then continuing to loop will still result in an error because resp.Size will keep increasing. In this case, the error won't be thrown, but adding logs will keep printing out, and the client may never receive resp. Unless we can guarantee that within maxSyncRegionBatchSize, resp.Size will never exceed the burst (20MiB).

For the latter, it doesn't matter, because there is a select ctx.Done() inside the loop.

If we can't guarantee that the information of maxSyncRegionBatchSize regions is definitely less than 20MiB, it's best to check if resp.Size is close to 20MiB before continuing at L275.

pkg/mcs/resourcemanager/server/server.go

Signed-off-by: okJiang <819421878@qq.com>

cmd/pd-server/main.go

JmPotato · 2024-06-26T05:39:10Z

pkg/mcs/registry/registry.go

-		l.RegisterRESTHandler(h)
-		log.Info("restful API service registered successfully", zap.String("prefix", prefix), zap.String("service-name", name))
+		if err := l.RegisterRESTHandler(h); err != nil {
+			log.Error("register restful API service failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))


It is best to distinguish it from line 86.

The line numbers of log will be automatically differentiated, or do you have any suggestions on how to modify the log content?

JmPotato · 2024-06-26T05:39:40Z

pkg/mcs/resourcemanager/server/grpc_service.go

@@ -49,7 +49,7 @@ type dummyRestService struct{}

 func (dummyRestService) ServeHTTP(w http.ResponseWriter, _ *http.Request) {
 	w.WriteHeader(http.StatusNotImplemented)
-	w.Write([]byte("not implemented"))
+	w.Write([]byte("not implemented")) // nolint:errcheck


Ditto. Prefer using _ = w.Write([]byte("not implemented")).

JmPotato · 2024-06-26T05:40:06Z

pkg/mcs/scheduling/server/grpc_service.go

@@ -53,7 +53,7 @@ type dummyRestService struct{}

 func (dummyRestService) ServeHTTP(w http.ResponseWriter, _ *http.Request) {
 	w.WriteHeader(http.StatusNotImplemented)
-	w.Write([]byte("not implemented"))
+	w.Write([]byte("not implemented")) // nolint:errcheck


Signed-off-by: okJiang <819421878@qq.com>

okJiang · 2024-06-27T02:51:02Z

Can you take a look again? Thanks! @JmPotato @rleungx @HuSharp

cmd/pd-server/main.go

rleungx · 2024-06-27T06:17:41Z

pkg/mcs/resourcemanager/server/grpc_service.go

+		if err := stream.Send(resps); err != nil {
+			return errors.WithStack(err)
+		}


I'm not sure whether it is on purpose.

indeed forgot to consider error handling previously.

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot · 2024-06-27T09:37:32Z

[LGTM Timeline notifier]

Timeline:

2024-06-27 07:06:19.518807219 +0000 UTC m=+876106.004296051: ☑️ agreed by rleungx.
2024-06-27 09:37:32.08624861 +0000 UTC m=+885178.571737443: ☑️ agreed by JmPotato.

JmPotato · 2024-06-27T09:40:01Z

/merge

ti-chi-bot · 2024-06-27T09:40:03Z

@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-27T09:40:12Z

/approve

ti-chi-bot · 2024-06-27T09:40:18Z

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

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.

rleungx · 2024-06-27T09:52:49Z

/approve

ti-chi-bot · 2024-06-27T10:19:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, niubell, 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]
~~pkg/mcs/resourcemanager/server/OWNERS~~ [niubell]
~~pkg/mcs/scheduling/server/config/OWNERS~~ [niubell]
~~pkg/mcs/tso/server/OWNERS~~ [niubell]
~~server/config/OWNERS~~ [niubell]

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

okJiang added 2 commits June 21, 2024 15:30

enable some files

2caf2f6

Signed-off-by: okJiang <819421878@qq.com>

enable server/*.go execpt server/api/*

cf68b06

Signed-off-by: okJiang <819421878@qq.com>

rleungx reviewed Jun 21, 2024

View reviewed changes

cmd/pd-server/main.go Outdated Show resolved Hide resolved

rleungx reviewed Jun 21, 2024

View reviewed changes

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Jun 21, 2024

okJiang added 2 commits June 21, 2024 17:20

fix comment

efff883

Signed-off-by: okJiang <819421878@qq.com>

fix comment

54edee8

Signed-off-by: okJiang <819421878@qq.com>

ti-chi-bot bot requested review from HuSharp and JmPotato June 24, 2024 03:32

rleungx reviewed Jun 24, 2024

View reviewed changes

HuSharp reviewed Jun 24, 2024

View reviewed changes

pkg/mcs/resourcemanager/server/server.go Show resolved Hide resolved

fix comment

c01b053

Signed-off-by: okJiang <819421878@qq.com>

okJiang mentioned this pull request Jun 25, 2024

*: enable errcheck for schedulers #8322

Merged

okJiang added 2 commits June 26, 2024 11:47

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

161f282

Signed-off-by: okJiang <819421878@qq.com>

update .golangci.yml

2f439eb

Signed-off-by: okJiang <819421878@qq.com>

JmPotato reviewed Jun 26, 2024

View reviewed changes

fix comment

8d8a416

Signed-off-by: okJiang <819421878@qq.com>

rleungx reviewed Jun 27, 2024

View reviewed changes

cmd/pd-server/main.go Outdated Show resolved Hide resolved

rleungx reviewed Jun 27, 2024

View reviewed changes

fix comment

2b411f5

Signed-off-by: okJiang <819421878@qq.com>

rleungx approved these changes Jun 27, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 27, 2024

Merge branch 'master' into enable-errcheck-3

1e31c68

JmPotato approved these changes Jun 27, 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 27, 2024

Merge branch 'master' into enable-errcheck-3

8b5a8c3

niubell approved these changes Jun 27, 2024

View reviewed changes

ti-chi-bot bot added the approved label Jun 27, 2024

ti-chi-bot bot merged commit 4048cba into tikv:master Jun 27, 2024
17 checks passed

okJiang deleted the enable-errcheck-3 branch June 27, 2024 10:23

HuSharp mentioned this pull request Aug 30, 2024

mcs/metastorage: remove the register restful API #8583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

okJiang commented Jun 21, 2024 •

edited

Loading

rleungx Jun 21, 2024

okJiang Jun 21, 2024

rleungx Jun 21, 2024

okJiang Jun 21, 2024

rleungx Jun 21, 2024

okJiang commented Jun 24, 2024

rleungx Jun 24, 2024

rleungx Jun 24, 2024

okJiang Jun 25, 2024

rleungx Jun 24, 2024

okJiang Jun 24, 2024

JmPotato Jun 26, 2024

okJiang Jun 26, 2024

JmPotato Jun 26, 2024

JmPotato Jun 26, 2024

okJiang commented Jun 27, 2024

rleungx Jun 27, 2024

nolouch Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

JmPotato commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

JmPotato commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

rleungx commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

	log.Error("register REST handler failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))
	log.Error("register restful API service failed", zap.String("prefix", prefix), zap.String("service-name", name), zap.Error(err))

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

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

Conversation

okJiang commented Jun 21, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 27, 2024

[LGTM Timeline notifier]

JmPotato commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

JmPotato commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

rleungx commented Jun 27, 2024

ti-chi-bot bot commented Jun 27, 2024

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

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

okJiang commented Jun 21, 2024 •

edited

Loading