Support adaptive update interval for low resolution ts #1484

MyonKeminta · 2024-11-01T02:01:23Z

Ref: pingcap/tidb#56809

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2024-11-06T11:29:34Z

oracle/oracle.go

+	// time.
+	// WARNING: This method does not guarantee whether the generated timestamp is legal for accessing the data.
+	// Neither is it safe to use it for verifying the legality of another calculated timestamp.
+	// Be sure to validate the timestamp before using it to access the data.


How about deprecating the usages of this interface in the future, or integrate the check of ValidateSnapshotReadTS into it?

It's more robust to return a verifed timestamp.

I've considered this. It makes sence, but it can't be easily done due to the current usage in TiDB repo.

Can we modify GetStaleTimestamp to return the latest fetched timestamp, without adding the arrival duration?

After discussion with @MyonKeminta , the refactor done could be done in another PR as it requires a lot of work.

Can we modify GetStaleTimestamp to return the latest fetched timestamp, without adding the arrival duration?

What you said is exactly what GetLowResolutionTSO does. If we deprecate the GetStaleTimestamp method, the change can be complictated and difficult considering the current usage in TiDB repo. Simply changing all usages of GetStaleTimestamp to GetLowResolutionTSO may significantly reduce the precision of user specified staleness.

oracle/oracles/pd.go

cfzjywxk · 2024-11-06T12:05:45Z

oracle/oracles/pd.go

+
+	adaptiveUpdateIntervalState struct {
+		// The mutex to avoid racing between updateTS goroutine and SetLowResolutionTimestampUpdateInterval.
+		mu sync.Mutex


How about moving the to be protected fields into the mu like

mu { var1 var2 }

But it's actually not protected. There are also some accesses without locking.🤔

oracle/oracles/pd.go

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

crazycs520 · 2024-11-07T07:08:33Z

oracle/oracles/pd.go

+			o.adaptiveLastTSUpdateInterval.Store(int64(configuredInterval))
+		}
+		if o.adaptiveUpdateIntervalState.state != adaptiveUpdateTSIntervalStateUnadjustable {
+			logutil.Logger(context.Background()).Info("update low resolution ts interval is not being adaptive because the configured interval is too short",


Suggested change

logutil.Logger(context.Background()).Info("update low resolution ts interval is not being adaptive because the configured interval is too short",

logutil.BgLogger().Info("update low resolution ts interval is not being adaptive because the configured interval is too short",

crazycs520 · 2024-11-07T07:15:35Z

oracle/oracles/pd.go

+			prevAdaptiveInterval := currentAdaptiveInterval
+			currentAdaptiveInterval = max(requiredStaleness-adaptiveUpdateTSIntervalShrinkingPreserve, minAllowedAdaptiveUpdateTSInterval)
+			o.adaptiveLastTSUpdateInterval.Store(int64(currentAdaptiveInterval))
+			logutil.Logger(context.Background()).Info("shrink low resolution ts update interval immediately",


crazycs520 · 2024-11-07T07:21:49Z

oracle/oracles/pd.go

+				zap.Duration("requestedStaleness", requiredStaleness),
+				zap.Duration("prevAdaptiveUpdateInterval", prevAdaptiveInterval),


why those log field names don't match the field name...

crazycs520 · 2024-11-07T09:04:32Z

oracle/oracles/pd.go

+
+func (o *pdOracle) getCurrentTSForValidation(ctx context.Context, opt *oracle.Option) (uint64, error) {
+	ch := o.tsForValidation.DoChan(opt.TxnScope, func() (interface{}, error) {
+		//metrics.ValidateReadTSFromPDCount.Inc()


debug code?

Ah, there should be a metrics here.

crazycs520 · 2024-11-07T09:21:33Z

oracle/oracles/pd.go

+		// current ctx.
+		res, err := o.GetTimestamp(context.Background(), opt)
+		// After finishing the current call, allow the next call to trigger fetching a new TS.
+		o.tsForValidation.Forget(opt.TxnScope)


No need to call Forget?

No it's not needed. I just realized that I misunderstood the usage of singleflight.

crazycs520 · 2024-11-07T09:32:33Z

oracle/oracles/pd.go

+		// If the call that triggers the execution of this function is canceled by the context, other calls that are
+		// waiting for reusing the same result should not be canceled. So pass context.Background() instead of the
+		// current ctx.
+		res, err := o.GetTimestamp(context.Background(), opt)


Suggested change

res, err := o.GetTimestamp(context.Background(), opt)

res, err := o.GetTimestamp(ctx, opt)

Using context.Background() here is expected, as said in the above comments.

crazycs520 · 2024-11-07T12:04:07Z

oracle/oracles/pd.go

+		estimatedCurrentTS, err := o.getStaleTimestamp(opt.TxnScope, 0)
+		if err != nil {
+			logutil.Logger(ctx).Warn("failed to estimate current ts by getSlateTimestamp for auto-adjusting update low resolution ts interval",
+				zap.Error(err), zap.Uint64("readTS", readTS), zap.String("txnScope", opt.TxnScope))
+		} else {
+			o.adjustUpdateLowResolutionTSIntervalWithRequestedStaleness(readTS, estimatedCurrentTS, time.Now())
+		}


I'm sorry, I don't understand why we need those logic?

We estimate the gap between the current time and the readTS, to see if the gap is shorter than the update interval, so that we need to shrink the update interval.

crazycs520 · 2024-11-07T12:06:45Z

oracle/oracle.go

+	// time.
+	// WARNING: This method does not guarantee whether the generated timestamp is legal for accessing the data.
+	// Neither is it safe to use it for verifying the legality of another calculated timestamp.
+	// Be sure to validate the timestamp before using it to access the data.


Can we modify GetStaleTimestamp to return the latest fetched timestamp, without adding the arrival duration?

crazycs520 · 2024-11-07T12:48:59Z

oracle/oracles/pd.go

+
+	configuredInterval := time.Duration(o.lastTSUpdateInterval.Load())
+	currentAdaptiveInterval := time.Duration(o.adaptiveLastTSUpdateInterval.Load())
+	if configuredInterval <= minAllowedAdaptiveUpdateTSInterval {


If configuredInterval <= minAllowedAdaptiveUpdateTSInterval is true, then adaptiveLastTSUpdateInterval can be set to configuredInterval. The variable minAllowedAdaptiveUpdateTSInterval doesn't behave like its name.

How about defining a minUpdateTSInterval and both lastTSUpdateInterval and adaptiveLastTSUpdateInterval should less than minUpdateTSInterval.

The name minAllowedAdaptiveUpdateTSInterval expected to limit the adaptive update ts interval, to avoid it automatically performs the update too frequently. If the user configures a short interval intentionally, we adopt the user's choice.

By the way I tried to refine the code of the nextUpdateInterval function, which extracted each branches into small closures and unified the log printing. Please see if it's better than the previous one.

crazycs520 · 2024-11-07T12:53:40Z

oracle/oracles/pd.go

+const (
+	// minAllowedAdaptiveUpdateTSInterval is the lower bound of the adaptive update ts interval for avoiding an abnormal
+	// read operation causing the update interval to be too short.
+	minAllowedAdaptiveUpdateTSInterval = 500 * time.Millisecond


The minimal value of tidb variable tidb_low_resolution_tso_update_interval is 10ms, why minAllowedAdaptiveUpdateTSInterval is 500ms?

To avoid the adaptive update interval mechanism automatically and silently chooses a too low interval can causing too high frequency of getting ts from PD. But we do not reject it if the user configures it explicitly and the low update interval is just what the user expects.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2024-11-08T10:03:32Z

oracle/oracles/pd.go

+	nextState := func(checkFuncs ...func() (adaptiveUpdateTSIntervalState, time.Duration)) time.Duration {
+		for _, f := range checkFuncs {
+			state, newInterval := f()
+			if state == none {


Should it be returned here if the state is adaptiveUpdateTSIntervalStateUnadjustable instead of continue processing?

If state is not none, it should return at line 454

cfzjywxk

Rest LGTM

you06 · 2024-11-11T03:34:48Z

oracle/oracles/pd.go

+	latestTS, err := o.GetLowResolutionTimestamp(ctx, opt)
+	// If we fail to get latestTS or the readTS exceeds it, get a timestamp from PD to double-check.
+	// But we don't need to strictly fetch the latest TS. So if there are already concurrent calls to this function
+	// loading the latest TS, we can just reuse the same result to avoid too many concurrent GetTS calls.


The current implementation is enough I think. This is just a minor suggestion.

Use singleflight to reduce the GetTS calls is reasonable, but it's possible that a valid ts is invalid in this case.

thread 1 validate current_ts and send a get ts request, PD allocates its latest TSO and the response is slow.

thread 2 validate current_ts and the singleflight will reuse the thread 1's TSO.

The GetTS request of thread 1's is responded.

Then thread 2 can get a stale TSO from PD which increases the failure possibility.

Maybe an enhancement is to resend a singleflight GetTS request if readTS > latestTS to recheck the timestamp in this case.

ti-chi-bot · 2024-11-11T07:45:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [cfzjywxk,crazycs520]

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 · 2024-11-11T07:45:51Z

[LGTM Timeline notifier]

Timeline:

2024-11-08 11:13:25.792549727 +0000 UTC m=+9167.983418723: ☑️ agreed by cfzjywxk.
2024-11-11 07:45:51.012984421 +0000 UTC m=+255913.203853417: ☑️ agreed by crazycs520.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Draft implementation

590500a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2024

MyonKeminta mentioned this pull request Nov 1, 2024

*: Use strict validation for stale read ts & flashback ts pingcap/tidb#57050

Merged

13 tasks

MyonKeminta added 2 commits November 4, 2024 18:07

Refine interactions between goroutines; add logs

6979a6d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add tests

5a322d8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2024

MyonKeminta added 2 commits November 6, 2024 14:51

Cover configuration changes

59d5568

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add more tests

19c6546

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/adaptive-low-resolution-tso-update-interval branch from 56c7714 to 19c6546 Compare November 6, 2024 09:41

MyonKeminta changed the title ~~[WIP] Support adaptive update interval for low resolution ts~~ Support adaptive update interval for low resolution ts Nov 6, 2024

MyonKeminta marked this pull request as ready for review November 6, 2024 09:41

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 6, 2024

MyonKeminta added 2 commits November 6, 2024 17:43

fix lint

5a945e9

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

fix lint

7e9bae9

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk requested review from cfzjywxk, zyguan, you06, crazycs520 and ekexium November 6, 2024 10:58

cfzjywxk reviewed Nov 6, 2024

View reviewed changes

Adderss comments

aea7b19

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

crazycs520 reviewed Nov 7, 2024

View reviewed changes

MyonKeminta added 4 commits November 8, 2024 15:11

clear the code of nextInterval

96c6007

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address comments

0e9c8b8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add metrics

2fef49a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Adjust comments

385419d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk reviewed Nov 8, 2024

View reviewed changes

cfzjywxk requested a review from crazycs520 November 8, 2024 10:15

cfzjywxk approved these changes Nov 8, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 8, 2024

you06 reviewed Nov 11, 2024

View reviewed changes

crazycs520 approved these changes Nov 11, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 11, 2024

ti-chi-bot bot merged commit 23531ad into tikv:master Nov 11, 2024
12 checks passed

MyonKeminta deleted the m/adaptive-low-resolution-tso-update-interval branch November 11, 2024 09:31

MyonKeminta added a commit to MyonKeminta/client-go that referenced this pull request Nov 11, 2024

Support adaptive update interval for low resolution ts (tikv#1484)

0460a4f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta mentioned this pull request Nov 11, 2024

Support adaptive update interval for low resolution ts (#1484) #1491

Merged

ti-chi-bot bot pushed a commit that referenced this pull request Nov 11, 2024

Support adaptive update interval for low resolution ts (#1484) (#1491)

e103358

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

This was referenced Nov 12, 2024

*: Use strict validation for stale read ts & flashback ts (#57050) pingcap/tidb#57315

Merged

*: Use strict validation for stale read ts & flashback ts (#57050) pingcap/tidb#57372

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support adaptive update interval for low resolution ts #1484

Support adaptive update interval for low resolution ts #1484

MyonKeminta commented Nov 1, 2024

cfzjywxk Nov 6, 2024

MyonKeminta Nov 7, 2024

crazycs520 Nov 7, 2024

cfzjywxk Nov 8, 2024

MyonKeminta Nov 8, 2024

cfzjywxk Nov 6, 2024

MyonKeminta Nov 7, 2024

crazycs520 Nov 7, 2024

crazycs520 Nov 7, 2024

crazycs520 Nov 7, 2024

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

crazycs520 Nov 7, 2024

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

MyonKeminta Nov 8, 2024 •

edited

Loading

crazycs520 Nov 7, 2024

MyonKeminta Nov 8, 2024

cfzjywxk Nov 8, 2024

MyonKeminta Nov 8, 2024

cfzjywxk left a comment

you06 Nov 11, 2024

ti-chi-bot bot commented Nov 11, 2024

ti-chi-bot bot commented Nov 11, 2024

	logutil.Logger(context.Background()).Info("update low resolution ts interval is not being adaptive because the configured interval is too short",
	logutil.BgLogger().Info("update low resolution ts interval is not being adaptive because the configured interval is too short",

		zap.Duration("requestedStaleness", requiredStaleness),
		zap.Duration("prevAdaptiveUpdateInterval", prevAdaptiveInterval),

	res, err := o.GetTimestamp(context.Background(), opt)
	res, err := o.GetTimestamp(ctx, opt)

Support adaptive update interval for low resolution ts #1484

Support adaptive update interval for low resolution ts #1484

Conversation

MyonKeminta commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 11, 2024

ti-chi-bot bot commented Nov 11, 2024

[LGTM Timeline notifier]

MyonKeminta Nov 8, 2024 •

edited

Loading