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.

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

client: print more info when leader disconnect #7907

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

CabinfeverB
Copy link
Member

@CabinfeverB CabinfeverB commented Mar 12, 2024

What problem does this PR solve?

Issue Number: Close #7906

should be merged after #7911

What is changed and how does it work?

Check List

Tests

  • Unit test

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Copy link
Contributor

ti-chi-bot bot commented Mar 12, 2024

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 12, 2024
@ti-chi-bot ti-chi-bot bot requested review from JmPotato and rleungx March 12, 2024 09:23
@CabinfeverB CabinfeverB requested review from HuSharp and removed request for JmPotato and rleungx March 12, 2024 09:23
@ti-chi-bot ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 12, 2024
leaderClient := newPDServiceClient(addr, addr, c.tlsCfg, newConn, true)
c.leader.Store(leaderClient)
if addr != oldLeader.GetAddress() {
change = true
Copy link
Member

Choose a reason for hiding this comment

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

It might fail to connect to the leader?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but the leader does change and it will affect the follower proxy

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Merging #7907 (6700333) into master (c1eabda) will increase coverage by 0.28%.
Report is 2 commits behind head on master.
The diff coverage is 87.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7907      +/-   ##
==========================================
+ Coverage   73.32%   73.60%   +0.28%     
==========================================
  Files         435      436       +1     
  Lines       48195    48349     +154     
==========================================
+ Hits        35337    35586     +249     
+ Misses       9784     9718      -66     
+ Partials     3074     3045      -29     
Flag Coverage Δ
unittests 73.60% <87.50%> (+0.28%) ⬆️

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
} else {
log.Warn("[pd] failed to connect leader", zap.String("leader", url), errs.ZapError(err))
}
if change {
// Set PD leader and Global TSO Allocator (which is also the PD leader)
leaderClient := newPDServiceClient(url, url, newConn, true)
Copy link
Member

@HuSharp HuSharp Mar 13, 2024

Choose a reason for hiding this comment

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

Does newConn will be nill? Do we need to change if change to if change || newConn != nil ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if the connection is not established, the leader is still updated.
The reason is that the leader has transferred to another PD server, and the client should update the member information. Meanwhile, the client must know the newest leader for follower proxy.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
return true, err
// Run callbacks
if c.tsoGlobalAllocLeaderUpdatedCb != nil {
err = multierr.Append(err, c.tsoGlobalAllocLeaderUpdatedCb(url))
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return the 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.

I am not sure if it is necessary to return both connection establishment and call back errors

Copy link
Member

Choose a reason for hiding this comment

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

The log already print it.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB CabinfeverB changed the title client: print more info when leader disconnect client: print more info when leader disconnect and avoid panic Mar 13, 2024
@CabinfeverB CabinfeverB changed the title client: print more info when leader disconnect and avoid panic client: print more info when leader disconnect Mar 13, 2024
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Comment on lines +1006 to +1007
if url != oldLeader.GetURL() {
change = true
Copy link
Member

Choose a reason for hiding this comment

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

This check and introducing change seem unnecessary due to line 999.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the client continues to fail to establish a connection with the leader, there will be situations where oldURL == url and err != nil``. So it's not always change = true` after line 999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log more info when client can not connect to leader
4 participants