Skip to content

concurrency_manager: make max-ts checker more robust#18080

Merged
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
ekexium:robust-max-ts-checker
Jan 6, 2025
Merged

concurrency_manager: make max-ts checker more robust#18080
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
ekexium:robust-max-ts-checker

Conversation

@ekexium
Copy link
Contributor

@ekexium ekexium commented Jan 2, 2025

What is changed and how it works?

Issue Number: ref #18055

What's Changed:

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Release note

None

PD TSO.
This reduces both false positive and false negative.

Signed-off-by: ekexium <eke@fastmail.com>
@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 2, 2025
Signed-off-by: ekexium <eke@fastmail.com>
@ekexium ekexium force-pushed the robust-max-ts-checker branch from 19e320a to f2de4da Compare January 2, 2025 11:10
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 2, 2025
@ekexium ekexium requested review from cfzjywxk and you06 January 2, 2025 11:11
@ekexium ekexium changed the title concurrency_manager: make max ts checker more robust concurrency_manager: make max-ts checker more robust Jan 2, 2025
@cfzjywxk cfzjywxk requested a review from MyonKeminta January 2, 2025 12:09
tso_confirmed: bool,
) -> Result<(), InvalidMaxTsUpdate> {
let can_panic = !using_approximate;
error!("invalid max_ts update";
Copy link
Collaborator

Choose a reason for hiding this comment

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

The WARN log level could be used if the error is not confirmed. Usually WARN indicates there could be errors, and ERROR unexpected error already happens and the system may not run in a normal state.

Signed-off-by: ekexium <eke@fastmail.com>
@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 3, 2025
Signed-off-by: ekexium <eke@fastmail.com>
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, you06

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:

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 ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 6, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 6, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-01-03 07:20:07.270365571 +0000 UTC m=+680542.626370114: ☑️ agreed by cfzjywxk.
  • 2025-01-06 06:03:36.301049832 +0000 UTC m=+160759.589881544: ☑️ agreed by you06.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 6, 2025

@ekexium: Your PR was out of date, I have automatically updated it for you.

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.

Details

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.

Signed-off-by: ekexium <eke@fastmail.com>
@ekexium
Copy link
Contributor Author

ekexium commented Jan 6, 2025

/retest

@ti-chi-bot ti-chi-bot bot merged commit ae60091 into tikv:master Jan 6, 2025
1 check passed
@ti-chi-bot ti-chi-bot bot added this to the Pool milestone Jan 6, 2025
ekexium added a commit to ti-chi-bot/tikv that referenced this pull request Jan 6, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ekexium added a commit to ti-chi-bot/tikv that referenced this pull request Jan 7, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ekexium added a commit to ti-chi-bot/tikv that referenced this pull request Jan 7, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ekexium added a commit to ti-chi-bot/tikv that referenced this pull request Jan 7, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
ekexium added a commit to ti-chi-bot/tikv that referenced this pull request Jan 7, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
xzhangxian1008 pushed a commit to xzhangxian1008/tikv that referenced this pull request Feb 8, 2025
ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Signed-off-by: xzhangxian1008 <xzhangxian@foxmail.com>
bufferflies pushed a commit to bufferflies/tikv that referenced this pull request Sep 19, 2025
* concurrency_manager: check update_max_ts against a limit (tikv#17917)

close tikv#17916

concurrency_manager: add safety boundary for max_ts updates

Add `max_ts_limit` to prevent unreasonable timestamp updates. The limit is
synchronized with PD timestamp periodically. Configure via max_ts_allowance_secs
 and max_ts_sync_interval_secs.

Updates from PD bypass this limit.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Signed-off-by: ekexium <eke@fastmail.com>

* concurrency_manager: double check via PD TSO before reporting error of invalid max-ts update (tikv#18057)

close tikv#18055

concurrency_manager: double check via PD TSO before reporting error of invalid max-ts update

Signed-off-by: ekexium <eke@fastmail.com>

* concurrency_manager: make max-ts checker more robust (tikv#18080)

ref tikv#18055

When validating max-ts updates, do not report error or panic unless confirmed by PD TSO.
This reduces both false positive and false negative cases.

Signed-off-by: ekexium <eke@fastmail.com>

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Signed-off-by: ekexium <eke@fastmail.com>

* config: rename config items for max-ts checker (tikv#18118)

ref tikv#17916

config: rename config items for max-ts checker

Signed-off-by: ekexium <eke@fastmail.com>

* concurrency-manager: do not assert in concurrency manager (tikv#18329)

ref tikv#17916

Do not assert in concurrency manager.

Signed-off-by: ekexium <eke@fastmail.com>

* delete unexpected files from cherry-picking

Signed-off-by: ekexium <eke@fastmail.com>

---------

Signed-off-by: ekexium <eke@fastmail.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

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants