*: improve the linter and fix some bugs #8015

rleungx · 2024-04-02T10:27:40Z

What problem does this PR solve?

Issue Number: Close #8019.

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2024-04-02T10:27:43Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
JmPotato

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.

rleungx · 2024-04-02T10:36:34Z

client/resource_group/controller/controller.go

@@ -386,8 +386,7 @@ func (c *ResourceGroupsController) Start(ctx context.Context) {
 				}

 			case gc := <-c.tokenBucketUpdateChan:
-				now := gc.run.now
-				go gc.handleTokenBucketUpdateEvent(c.loopCtx, now)
+				go gc.handleTokenBucketUpdateEvent(c.loopCtx)


now is not used, we need to confirm if it is expected.

It's okay to remove it

rleungx · 2024-04-02T10:37:29Z

client/tso_dispatcher.go

@@ -797,6 +797,7 @@ func (c *tsoClient) processRequests(
 	stream tsoStream, dcLocation string, tbc *tsoBatchController,
 ) error {
 	requests := tbc.getCollectedRequests()
+	// nolint


defer is in the loop, not sure if it is right.

rleungx · 2024-04-02T10:38:36Z

pkg/mcs/resourcemanager/server/config.go

@@ -108,7 +108,8 @@ type ControllerConfig struct {
 }

 // Adjust adjusts the configuration and initializes it with the default value if necessary.
-func (rmc *ControllerConfig) Adjust(meta *configutil.ConfigMetaData) {
+// FIXME: is it expected?
+func (rmc *ControllerConfig) Adjust(_ *configutil.ConfigMetaData) {


Adjust doesn't use meta, is it expected?

IMO, it's better use meta for the method Adjust of RequestUnitConfig.

rleungx · 2024-04-02T10:39:41Z

pkg/schedule/checker/rule_checker.go

@@ -347,7 +347,7 @@ func (c *RuleChecker) fixLooseMatchPeer(region *core.RegionInfo, fit *placement.
 	if region.GetLeader().GetId() != peer.GetId() && rf.Rule.Role == placement.Leader {
 		ruleCheckerFixLeaderRoleCounter.Inc()
 		if c.allowLeader(fit, peer) {
-			return operator.CreateTransferLeaderOperator("fix-leader-role", c.cluster, region, region.GetLeader().GetStoreId(), peer.GetStoreId(), []uint64{}, 0)
+			return operator.CreateTransferLeaderOperator("fix-leader-role", c.cluster, region, peer.GetStoreId(), []uint64{}, 0)


transfer leader doesn't need a source store id

It isn't necessary for source.

rleungx · 2024-04-02T10:41:00Z

pkg/utils/tsoutil/tso_request.go

@@ -32,7 +32,7 @@ type Request interface {
 	getCount() uint32
 	// process sends request and receive response via stream.
 	// count defines the count of timestamps to retrieve.
-	process(forwardStream stream, count uint32, tsoProtoFactory ProtoFactory) (tsoResp, error)
+	process(forwardStream stream, count uint32) (tsoResp, error)


we don't use factory in process

rleungx · 2024-04-02T10:41:36Z

server/grpc_service.go

@@ -556,7 +556,7 @@ func (s *GrpcServer) Tso(stream pdpb.PD_TsoServer) error {

 			if errCh == nil {
 				doneCh = make(chan struct{})
-				defer close(doneCh)
+				defer close(doneCh) // nolint


defer here is not right.

rleungx · 2024-04-02T10:44:04Z

pkg/utils/testutil/api_check.go

@@ -100,7 +100,8 @@ func ReadGetJSONWithBody(re *require.Assertions, client *http.Client, url string
 	if err != nil {
 		return err
 	}
-	return checkResp(resp, StatusOK(re), ExtractJSON(re, data))
+	checkOpts = append(checkOpts, StatusOK(re), ExtractJSON(re, data))


previously this function was not right

rleungx · 2024-04-02T10:44:57Z

pkg/schedule/splitter/region_splitter.go

@@ -118,7 +119,7 @@ func (r *RegionSplitter) splitRegionsByKeys(parCtx context.Context, splitKeys []
 				r.handler.ScanRegionsByKeyRange(groupKeys, results)
 			}
 		case <-ctx.Done():
-			break
+			break outerLoop


it won't exit the for loop

codecov · 2024-04-03T03:30:42Z

Codecov Report

Merging #8015 (14503e3) into master (67461dd) will increase coverage by 0.04%.
The diff coverage is 76.01%.

❗ Current head 14503e3 differs from pull request most recent head 013fbc4. Consider uploading reports for the commit 013fbc4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8015      +/-   ##
==========================================
+ Coverage   77.27%   77.32%   +0.04%     
==========================================
  Files         468      468              
  Lines       60890    60868      -22     
==========================================
+ Hits        47055    47068      +13     
+ Misses      10291    10261      -30     
+ Partials     3544     3539       -5

Flag	Coverage Δ
unittests	`77.32% <76.01%> (+0.04%)`	⬆️

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

rleungx · 2024-04-03T03:35:04Z

pkg/schedule/splitter/region_splitter_test.go

@@ -76,7 +76,7 @@ func (suite *regionSplitterTestSuite) SetupSuite() {
 	suite.ctx, suite.cancel = context.WithCancel(context.Background())
 }

-func (suite *regionSplitterTestSuite) TearDownTest() {
+func (suite *regionSplitterTestSuite) TearDownSuite() {


We need use TearDownSuite instead of TearDownTest

JmPotato · 2024-04-07T02:16:05Z

Makefile

@@ -181,9 +181,6 @@ static: install-tools pre-build
 	@ gofmt -s -l -d $(PACKAGE_DIRECTORIES) 2>&1 | awk '{ print } END { if (NR > 0) { exit 1 } }'
 	@ echo "golangci-lint ..."
 	@ golangci-lint run --verbose $(PACKAGE_DIRECTORIES) --allow-parallel-runners
-	@ echo "revive ..."
-	@ revive -formatter friendly -config revive.toml $(PACKAGES)


Do we need to remove revive.toml also?

CabinfeverB · 2024-04-07T03:07:54Z

client/tso_dispatcher.go

@@ -797,6 +797,7 @@ func (c *tsoClient) processRequests(
 	stream tsoStream, dcLocation string, tbc *tsoBatchController,
 ) error {
 	requests := tbc.getCollectedRequests()
+	// nolint


Maybe we can create a method to handle batch trace operations

ti-chi-bot · 2024-04-07T19:06:06Z

PR needs rebase.

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 kubernetes/test-infra repository.

Signed-off-by: Ryan Leung <[email protected]>

HuSharp · 2024-04-11T08:49:25Z

/merge

ti-chi-bot · 2024-04-11T08:49:27Z

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

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2024-04-11T08:49:29Z

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

Commit hash: 14503e3

ti-chi-bot · 2024-04-11T08:49:42Z

@rleungx: 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.

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 2, 2024

ti-chi-bot bot requested review from nolouch and Yisaer April 2, 2024 10:28

rleungx commented Apr 2, 2024

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Apr 3, 2024

rleungx commented Apr 3, 2024

View reviewed changes

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

JmPotato reviewed Apr 7, 2024

View reviewed changes

rleungx force-pushed the improve-linter branch from f9b8944 to bd69d58 Compare April 7, 2024 02:27

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2024

CabinfeverB reviewed Apr 7, 2024

View reviewed changes

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2024

rleungx force-pushed the improve-linter branch from 9f34462 to a1ec690 Compare April 10, 2024 08:38

JmPotato approved these changes Apr 10, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 10, 2024

rleungx added 7 commits April 11, 2024 11:11

improve the linter and fix some bugs

9c48e6b

Signed-off-by: Ryan Leung <[email protected]>

fix suite

d081d95

Signed-off-by: Ryan Leung <[email protected]>

remove revive toml

07f5c94

Signed-off-by: Ryan Leung <[email protected]>

fix more

5748dca

Signed-off-by: Ryan Leung <[email protected]>

fix rc config

b0ca2eb

Signed-off-by: Ryan Leung <[email protected]>

fix test

302b96b

Signed-off-by: Ryan Leung <[email protected]>

fix conflicts

ff5110f

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the improve-linter branch from a1ec690 to ff5110f Compare April 11, 2024 03:12

fix

14503e3

Signed-off-by: Ryan Leung <[email protected]>

rleungx removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

rleungx requested review from HuSharp and removed request for Yisaer April 11, 2024 03:38

HuSharp approved these changes Apr 11, 2024

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 11, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 11, 2024

Merge branch 'master' into improve-linter

013fbc4

ti-chi-bot bot merged commit 7e18a69 into tikv:master Apr 11, 2024
22 checks passed

rleungx deleted the improve-linter branch April 11, 2024 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: improve the linter and fix some bugs #8015

*: improve the linter and fix some bugs #8015

rleungx commented Apr 2, 2024 •

edited

Loading

ti-chi-bot bot commented Apr 2, 2024 •

edited

Loading

rleungx Apr 2, 2024

CabinfeverB Apr 7, 2024

rleungx Apr 2, 2024

rleungx Apr 2, 2024

CabinfeverB Apr 7, 2024

rleungx Apr 2, 2024

CabinfeverB Apr 7, 2024

rleungx Apr 2, 2024

rleungx Apr 2, 2024

rleungx Apr 2, 2024

rleungx Apr 2, 2024

codecov bot commented Apr 3, 2024 •

edited

Loading

rleungx Apr 3, 2024

JmPotato Apr 7, 2024

rleungx Apr 7, 2024

CabinfeverB Apr 7, 2024

ti-chi-bot bot commented Apr 7, 2024

HuSharp commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

*: improve the linter and fix some bugs #8015

*: improve the linter and fix some bugs #8015

Conversation

rleungx commented Apr 2, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Apr 2, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

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

ti-chi-bot bot commented Apr 7, 2024

HuSharp commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

ti-chi-bot bot commented Apr 11, 2024

rleungx commented Apr 2, 2024 •

edited

Loading

ti-chi-bot bot commented Apr 2, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading