server: advance ServerStart check by okJiang · Pull Request #8951 · tikv/pd

okJiang · 2024-12-26T06:55:55Z

What problem does this PR solve?

Issue Number: Close #8950

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

Release note

Fix the problem that PD may panic after enable `tidb_enable_tso_follower_proxy`.

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

codecov · 2024-12-26T07:03:13Z

Codecov Report

❌ Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.33%. Comparing base (0bfa31f) to head (6186e8c).
⚠️ Report is 530 commits behind head on master.

❌ Your patch status has failed because the patch coverage (53.84%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8951      +/-   ##
==========================================
- Coverage   76.34%   76.33%   -0.02%     
==========================================
  Files         465      465              
  Lines       70537    70547      +10     
==========================================
- Hits        53853    53850       -3     
- Misses      13344    13356      +12     
- Partials     3340     3341       +1

Flag	Coverage Δ
unittests	`∅ <53.84%> (∅)`

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

okJiang · 2024-12-27T07:43:07Z

/cc @rleungx @JmPotato @bufferflies

bufferflies · 2024-12-27T08:49:44Z

 			return nil, errs.ErrGRPCRateLimitExceeded(err)
 		}
 	}
+	if s.IsClosed() {


maybe placing this check to the head of this function is better?

Other functions' checking IsClosed() is inside unaryMiddleware. All of the unaryMiddleware is behind Ratelimit component. So here it is consistent with other places.

rleungx · 2024-12-30T10:18:05Z

 		}

+		// TSO uses leader lease to determine validity. No need to check leader here.
+		if s.IsClosed() {


How about adding a test?

Added 2bd964d

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

okJiang · 2024-12-31T10:01:26Z

/retest

okJiang · 2025-01-02T03:52:41Z

ping @bufferflies

bufferflies · 2024-12-30T02:24:32Z

 			return nil, errs.ErrGRPCRateLimitExceeded(err)
 		}
 	}
+	if s.IsClosed() {


ti-chi-bot · 2025-01-02T07:17:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bufferflies,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 · 2025-01-02T07:17:46Z

[LGTM Timeline notifier]

Timeline:

2024-12-30 10:16:56.864453078 +0000 UTC m=+345552.220457617: ☑️ agreed by rleungx.
2025-01-02 07:17:45.094083965 +0000 UTC m=+594000.450088530: ☑️ agreed by bufferflies.

ti-chi-bot · 2025-01-03T08:22:10Z

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

ti-chi-bot · 2025-01-03T08:22:10Z

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

ti-chi-bot · 2025-01-03T08:22:11Z

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

ti-chi-bot · 2025-01-03T08:22:12Z

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

close #8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

close tikv#8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

advance check

80a91af

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

add more

7fd2521

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

ti-chi-bot Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/needs-triage-completed labels Dec 26, 2024

ti-chi-bot Bot requested review from JmPotato, bufferflies and rleungx December 27, 2024 07:43

bufferflies reviewed Dec 27, 2024

View reviewed changes

rleungx approved these changes Dec 30, 2024

View reviewed changes

ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Dec 30, 2024

rleungx reviewed Dec 30, 2024

View reviewed changes

add a test

2bd964d

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

ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 31, 2024

okJiang added 3 commits December 31, 2024 16:49

Merge branch 'master' of github.com:tikv/pd into check-server-start

e168226

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

tools make tidy

9045323

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

update

0357ee1

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

bufferflies approved these changes Jan 2, 2025

View reviewed changes

Comment thread server/grpc_service.go

return nil, errs.ErrGRPCRateLimitExceeded(err)

}

}

if s.IsClosed() {

Copy link
Copy Markdown

Contributor

bufferflies Dec 30, 2024

Choose a reason for hiding this comment

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

ti-chi-bot Bot added the lgtm label Jan 2, 2025

ti-chi-bot Bot added 2 commits January 2, 2025 07:18

Merge branch 'master' into check-server-start

650f562

Merge branch 'master' into check-server-start

6186e8c

ti-chi-bot Bot merged commit 7a30ebc into tikv:master Jan 2, 2025

ti-chi-bot mentioned this pull request Jan 3, 2025

server: advance ServerStart check (#8951) #8969

Merged

This was referenced Jan 3, 2025

server: advance ServerStart check (#8951) #8970

Merged

server: advance ServerStart check (#8951) #8971

Merged

ti-chi-bot mentioned this pull request Jan 3, 2025

server: advance ServerStart check (#8951) #8972

Merged

okJiang deleted the check-server-start branch January 3, 2025 08:44

ti-chi-bot Bot pushed a commit that referenced this pull request Jan 6, 2025

server: advance ServerStart check (#8951) (#8969)

28001fd

close #8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

ti-chi-bot Bot pushed a commit that referenced this pull request Jan 6, 2025

server: advance ServerStart check (#8951) (#8971)

d62b7b6

close #8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

ti-chi-bot Bot pushed a commit that referenced this pull request Jan 6, 2025

server: advance ServerStart check (#8951) (#8972)

93d853f

close #8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

ti-chi-bot Bot pushed a commit that referenced this pull request Jan 6, 2025

server: advance ServerStart check (#8951) (#8970)

9c37e67

close #8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

okJiang added a commit to okJiang/pd that referenced this pull request Mar 10, 2025

server: advance ServerStart check (tikv#8951) (tikv#8971)

dc754aa

close tikv#8950 Signed-off-by: okJiang <819421878@qq.com> Co-authored-by: okJiang <819421878@qq.com>

Conversation

okJiang commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

codecov Bot commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

okJiang commented Dec 27, 2024

Uh oh!

bufferflies Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

okJiang Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

bufferflies Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

rleungx Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

okJiang Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

okJiang commented Dec 31, 2024

Uh oh!

okJiang commented Jan 2, 2025

Uh oh!

bufferflies Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Jan 2, 2025

Uh oh!

ti-chi-bot Bot commented Jan 2, 2025

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot commented Jan 3, 2025

Uh oh!

ti-chi-bot commented Jan 3, 2025

Uh oh!

ti-chi-bot commented Jan 3, 2025

Uh oh!

ti-chi-bot commented Jan 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

okJiang commented Dec 26, 2024 •

edited

Loading

codecov Bot commented Dec 26, 2024 •

edited

Loading