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

pick-6.5: Support adaptive update interval for low resolution ts (#1484) #1531

Merged
merged 10 commits into from
Jan 9, 2025

Conversation

ekexium
Copy link
Contributor

@ekexium ekexium commented Dec 19, 2024

Cherry pick #1484 and #1502 to tidb-6.5

@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 19, 2024
MyonKeminta and others added 6 commits January 6, 2025 19:08
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
@ekexium ekexium force-pushed the tidb-6.5-pick-1484 branch from b3f755a to 265712e Compare January 6, 2025 11:08
@ekexium ekexium requested review from you06 and MyonKeminta January 6, 2025 11:08
Copy link
Contributor

@you06 you06 left a comment

Choose a reason for hiding this comment

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

Also pick #1502 in this PR?

@ekexium
Copy link
Contributor Author

ekexium commented Jan 7, 2025

Also pick #1502 in this PR?

I initially planned to cherry-pick these changes in separate PRs. After taking a look now, since it's only 11 lines, let's combine them into one.

…nd there's too much logs about setting the config (tikv#1502)

Signed-off-by: MyonKeminta <[email protected]>
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jan 7, 2025
@@ -47,19 +48,111 @@ import (
"github.com/tikv/client-go/v2/metrics"
"github.com/tikv/client-go/v2/oracle"
pd "github.com/tikv/pd/client"
"go.uber.org/atomic"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to use the uber version rhe the original PR didn't? I think using the std one should be just fine🤔 Is this a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std atomic was introduced in go 1.19, while release-6.5 uses go 1.18.

Copy link
Contributor

Choose a reason for hiding this comment

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

😰

// pdOracle is an Oracle that uses a placement driver client as source.
type pdOracle struct {
c pd.Client
// txn_scope (string) -> lastTSPointer (*lastTSOPointer)
lastTSMap sync.Map
quit chan struct{}
// The configured interval to update the low resolution ts. Set by SetLowResolutionTimestampUpdateInterval.
// For TiDB, this is directly controlled by the system variable `tidb_low_resolution_tso_update_interval`.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can consider picking from other cherrypicks to old branches, in which I've modified some contents. e.g. #1491

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I've picked #1491 as the last commit.

Comment on lines 489 to 490
// Note that as `doUpdate` updates last tick time while `nextUpdateInterval` may perform calculation depending on the
// last tick time, `doUpdate` should be called after finishing calculating the next interval.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks this is part of #1502 and don't need to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes

…nd there's too much logs about setting the config (tikv#1502)

Signed-off-by: MyonKeminta <[email protected]>
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 9, 2025
Copy link

ti-chi-bot bot commented Jan 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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

Copy link

ti-chi-bot bot commented Jan 9, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-01-07 07:34:13.086801182 +0000 UTC m=+252596.375632890: ☑️ agreed by you06.
  • 2025-01-09 05:45:40.29846005 +0000 UTC m=+418883.587291752: ☑️ agreed by MyonKeminta.

@ti-chi-bot ti-chi-bot bot merged commit ccec7ef into tikv:tidb-6.5 Jan 9, 2025
10 checks passed
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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants