Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: Improve region forward #7305

Merged
merged 3 commits into from
Nov 14, 2023
Merged

*: Improve region forward #7305

merged 3 commits into from
Nov 14, 2023

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Nov 2, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: Ref #5839.

What is changed and how does it work?

Improve the region forwarding logic.

Check List

Tests

  • Manual test

Release note

None.

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 2, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • CabinfeverB
  • lhy1024

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.

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 2, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2023
@rleungx rleungx force-pushed the improve-region-forward branch 3 times, most recently from ee13cd4 to 0f0fa4b Compare November 7, 2023 07:02
@rleungx
improve region forward
b8885a7 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx rleungx marked this pull request as ready for review November 13, 2023 02:36
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2023
@rleungx rleungx requested review from lhy1024 and CabinfeverB and removed request for disksing November 13, 2023 02:38
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2023 •
edited
Loading

Codecov Report

Merging #7305 (f48ff19) into master (e6e35fd) will decrease coverage by 0.19%.
Report is 2 commits behind head on master.
The diff coverage is 62.50%.

❗ Current head f48ff19 differs from pull request most recent head f186c08. Consider uploading reports for the commit f186c08 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7305      +/-   ##
==========================================
- Coverage   74.55%   74.36%   -0.19%     
==========================================
  Files         447      451       +4     
  Lines       48666    48867     +201     
==========================================
+ Hits        36282    36341      +59     
- Misses       9189     9320     +131     
- Partials     3195     3206      +11
Flag Coverage Δ
unittests 74.36% <62.50%> (-0.19%) ⬇️

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

@rleungx
solve the conflicts
f48ff19 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
CabinfeverB
CabinfeverB approved these changes Nov 13, 2023
View reviewed changes
server/grpc_service.go Outdated Show resolved Hide resolved
server/forward.go
resp, err := s.forwardTSORequest(forwardCtx, request, forwardStream)
close(done)
if err != nil {
if strings.Contains(err.Error(), errs.NotLeaderErr) {
Copy link
Member

Choose a reason for hiding this comment

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

How about add a handleErr func?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't change the logic here, maybe we can keep it as it is.

@ti-chi-bot ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 13, 2023
lhy1024
lhy1024 reviewed Nov 14, 2023
View reviewed changes
server/forward.go
return conn.(*grpc.ClientConn), nil
}

func (s *GrpcServer) getForwardedHost(ctx, streamCtx context.Context, serviceName ...string) (forwardedHost string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (s *GrpcServer) getForwardedHost(ctx, streamCtx context.Context, serviceName ...string) (forwardedHost string, err error) {
func (s *GrpcServer) getForwardedHost(ctx, streamCtx context.Context, serviceNames ...string) (forwardedHost string, err error) {

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just an optional parameter in stead of plural.

@ti-chi-bot 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 Nov 14, 2023
@rleungx
Copy link
Member Author

rleungx commented Nov 14, 2023

/merge

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 14, 2023

@rleungx: 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 ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 14, 2023

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

Commit hash: f48ff19

@ti-chi-bot ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 14, 2023
@ti-chi-bot ti-chi-bot bot merged commit 8dcd497 into tikv:master Nov 14, 2023
22 checks passed
@rleungx rleungx deleted the improve-region-forward branch November 14, 2023 03:29
@rleungx rleungx mentioned this pull request Nov 30, 2023
Closed
rleungx added a commit to rleungx/pd that referenced this pull request Dec 1, 2023
@rleungx @ti-chi-bot
*: Improve region forward (tikv#7305)
70ae98f 
ref tikv#5839

Signed-off-by: Ryan Leung <rleungx@gmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhy1024 lhy1024 lhy1024 approved these changes

@CabinfeverB CabinfeverB CabinfeverB approved these changes

@JmPotato JmPotato Awaiting requested review from JmPotato

Assignees
No one assigned
Labels
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. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rleungx @lhy1024 @CabinfeverB