pd icon indicating copy to clipboard operation
pd copied to clipboard

server: Resign as leader on exit or SIGHUP

Open mjonss opened this issue 1 year ago • 8 comments

close tikv/pd#6344

What problem does this PR solve?

Issue Number: Close #6344

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

Code changes

Side effects

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

Related changes

Release note

None.

mjonss avatar Apr 19 '23 22:04 mjonss

[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 avatar Apr 19 '23 22:04 ti-chi-bot

Hi @mjonss. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ti-chi-bot avatar Apr 19 '23 22:04 ti-chi-bot

I'm not sure if this patch is intended to solve the query latency spike issue caused by rolling restart of PD. If so, this patch may not solve the problem.

We conducted an experiment by simulating the rolling update process of PD using the command /pd-ctl member leader transfer manually. We made sure that the leadership of a PD was transferred to another PD WITHOUT shutting it down. However, we still observed 2-3 minute spikes in P100 TiDB query latency.

Screenshot 2023-04-24 at 9 02 01 AM

This experiment demonstrates that even with graceful shutdown, re-election still occurs and it impacts TiDB queries. This PR should only save the time for PD to realize there is no leader (etcd election timeout, by default it's 1 second).

cc: @nolouch

coderplay avatar Apr 24 '23 16:04 coderplay

I'm not sure if this patch is intended to solve the query latency spike issue caused by rolling restart of PD. If so, this patch may not solve the problem.

We conducted an experiment by simulating the rolling update process of PD using the command /pd-ctl member leader transfer manually. We made sure that the leadership of a PD was transferred to another PD WITHOUT shutting it down. However, we still observed 2-3 minute spikes in P100 TiDB query latency.

This experiment demonstrates that even with graceful shutdown, re-election still occurs and it impacts TiDB queries. This PR should only save the time for PD to realize there is no leader (etcd election timeout, by default it's 1 second).

cc: @nolouch

@coderplay Is it only a P100/long tail latency, or all TiDB queries that are affected in your experiment?

mjonss avatar Apr 24 '23 17:04 mjonss

@coderplay Is it only a P100/long tail latency, or all TiDB queries that are affected in your experiment?

This is P100/long tail latency, no obvious spikes on P95.

Theoretically, this PR won't avoid the leader re-election process, right?

The best approach would be to make the PD leader cutover an atomic process. The old leader should still serve as a leader and continue to serve TSO until the cutover occurs. However, there is a detail here. PD's clients -- TiDB/TiKV, are not aware of this atomic cutover. They will only query the new leader's address after a failure.

coderplay avatar Apr 24 '23 18:04 coderplay

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

ti-chi-bot[bot] avatar May 02 '23 17:05 ti-chi-bot[bot]

After looking at the PD code, the pd-ctl transfer leader and the resign in this PR are actually equivalent. Resign simply allows PD to randomly select another PD as the new leader. It actually avoids the Etcd election timeout and re-election process, while a non-graceful shutdown does NOT. In another experiment, we found that although this PR still has some blips in P100 latency, P999 is not affected, while non-graceful shutdowns have obvious blips in P999.

so, +1 for the patch.

coderplay avatar May 02 '23 18:05 coderplay

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.

ti-chi-bot[bot] avatar Feb 03 '24 08:02 ti-chi-bot[bot]