controller: avoid the invalid nil pointer #6486

nolouch · 2023-05-18T10:34:35Z

What problem does this PR solve?

Issue Number: Close #6485

What is changed and how does it work?

controller: avoid the invalid nil pointer

Check List

Tests

Unit test
Integration test
ref case Hit connection running loop panic in tidb when do patrol check in serverless prod env #6485

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.

ti-chi-bot · 2023-05-18T10:34:37Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
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.

Signed-off-by: nolouch <nolouch@gmail.com>

codecov · 2023-05-18T10:51:58Z

Codecov Report

Patch coverage: 40.42% and project coverage change: +0.04 🎉

Comparison is base (6f6bf01) 75.03% compared to head (08db261) 75.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6486      +/-   ##
==========================================
+ Coverage   75.03%   75.07%   +0.04%     
==========================================
  Files         410      410              
  Lines       41823    41858      +35     
==========================================
+ Hits        31380    31424      +44     
+ Misses       7698     7687      -11     
- Partials     2745     2747       +2

Flag	Coverage Δ
unittests	`75.07% <40.42%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
client/resource_group/controller/model.go	`58.33% <7.14%> (-15.36%)`	⬇️
client/client.go	`62.67% <88.23%> (+3.42%)`	⬆️
client/keyspace_client.go	`65.15% <100.00%> (+1.51%)`	⬆️
tests/integrations/mcs/testutil.go	`96.93% <100.00%> (ø)`

... and 31 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: nolouch <nolouch@gmail.com>

client/resource_group/controller/model.go

JmPotato · 2023-05-18T13:23:27Z

client/resource_group/controller/model.go

 		return group.RawResourceSettings.IoWrite
 	}
 	return nil
 }

 func add(custom1 *rmpb.Consumption, custom2 *rmpb.Consumption) {
+	if custom1 == nil || custom2 == nil {
+		return
+	}


Is it reasonable to return the non-nil one and only return nothing when both are nil?

I'm not getting your point. It's reasonable to return once it has nil in here I think.

For example, if custom1 is nil and custom2 is not, we can simply return custom2 instead of a nil value.

It means if custom1 is nil, we can return custom2

There is some misunderstanding here. this function no return values, just exit the function call.

client/resource_group/controller/model.go

Signed-off-by: nolouch <nolouch@gmail.com>

nolouch · 2023-05-19T09:05:53Z

PTAL @JmPotato @CabinfeverB @HuSharp

CabinfeverB

Is it possible nil consumption occur inKVCalculator?
rest LGTM.

Signed-off-by: nolouch <nolouch@gmail.com>

nolouch · 2023-05-19T09:34:51Z

KVCalculator

Done @CabinfeverB

ti-chi-bot · 2023-05-19T09:35:58Z

@HuSharp: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

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.

nolouch · 2023-05-19T10:14:47Z

/merge

ti-chi-bot · 2023-05-19T10:14:49Z

@nolouch: 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 · 2023-05-19T10:14:50Z

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

Commit hash: 08db261

ti-chi-bot · 2023-05-19T10:16:14Z

In response to a cherrypick label: new pull request created to branch release-7.1: #6496.

ref #6485, close #6485, ref #6486 controller: avoid the invalid nil pointer Signed-off-by: nolouch <nolouch@gmail.com> Co-authored-by: nolouch <nolouch@gmail.com>

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2023

ti-chi-bot bot requested review from lhy1024 and Yisaer May 18, 2023 10:34

ti-chi-bot bot added needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. and removed do-not-merge/needs-triage-completed labels May 18, 2023

controller: avoid the invalid nil

e42c946

Signed-off-by: nolouch <nolouch@gmail.com>

nolouch force-pushed the fix-panic branch from 0ce8042 to e42c946 Compare May 18, 2023 10:37

add test

b4bf1ee

Signed-off-by: nolouch <nolouch@gmail.com>

nolouch requested review from JmPotato, CabinfeverB and Connor1996 and removed request for Yisaer and lhy1024 May 18, 2023 11:14

JmPotato reviewed May 18, 2023

View reviewed changes

HuSharp reviewed May 19, 2023

View reviewed changes

client/resource_group/controller/model.go Show resolved Hide resolved

add comments

e95cbf6

Signed-off-by: nolouch <nolouch@gmail.com>

CabinfeverB reviewed May 19, 2023

View reviewed changes

address comments

9a0909a

Signed-off-by: nolouch <nolouch@gmail.com>

HuSharp approved these changes May 19, 2023

View reviewed changes

JmPotato approved these changes May 19, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 19, 2023

Merge branch 'master' into fix-panic

08db261

CabinfeverB approved these changes May 19, 2023

View reviewed changes

ti-chi-bot bot removed the status/LGT1 Indicates that a PR has LGTM 1. label May 19, 2023

ti-chi-bot bot added the status/LGT2 Indicates that a PR has LGTM 2. label May 19, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 19, 2023

ti-chi-bot bot merged commit 1613e22 into tikv:master May 19, 2023

ti-chi-bot mentioned this pull request May 19, 2023

controller: avoid the invalid nil pointer (#6486) #6496

Merged

nolouch deleted the fix-panic branch May 19, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: avoid the invalid nil pointer #6486

controller: avoid the invalid nil pointer #6486

nolouch commented May 18, 2023

ti-chi-bot bot commented May 18, 2023 •

edited

Loading

codecov bot commented May 18, 2023 •

edited

Loading

JmPotato May 18, 2023

nolouch May 19, 2023

JmPotato May 19, 2023

CabinfeverB May 19, 2023

nolouch May 19, 2023 •

edited

Loading

CabinfeverB May 19, 2023

nolouch commented May 19, 2023

CabinfeverB left a comment

nolouch commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

nolouch commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

ti-chi-bot commented May 19, 2023

controller: avoid the invalid nil pointer #6486

controller: avoid the invalid nil pointer #6486

Conversation

nolouch commented May 18, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 18, 2023 • edited Loading

codecov bot commented May 18, 2023 • edited Loading

Codecov Report

JmPotato May 18, 2023

Choose a reason for hiding this comment

nolouch May 19, 2023

Choose a reason for hiding this comment

JmPotato May 19, 2023

Choose a reason for hiding this comment

CabinfeverB May 19, 2023

Choose a reason for hiding this comment

nolouch May 19, 2023 • edited Loading

Choose a reason for hiding this comment

CabinfeverB May 19, 2023

Choose a reason for hiding this comment

nolouch commented May 19, 2023

CabinfeverB left a comment

Choose a reason for hiding this comment

nolouch commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

nolouch commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

ti-chi-bot bot commented May 19, 2023

ti-chi-bot commented May 19, 2023

ti-chi-bot bot commented May 18, 2023 •

edited

Loading

codecov bot commented May 18, 2023 •

edited

Loading

nolouch May 19, 2023 •

edited

Loading