br: add retry for raw kv client put #58963

Tristan1900 · 2025-01-15T20:34:11Z

What problem does this PR solve?

Issue Number: close #58845

Problem Summary:

What changed and how does it work?

Adds a retry logic in raw kv client to mask temporary connection error from the client_go library

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Adds retry logic for raw kv client put operation when doing log meta restore to mask any temporary errors from the client_go (tikv) library

ti-chi-bot · 2025-01-15T20:34:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yujuncen for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

br/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tiprow · 2025-01-15T20:34:31Z

Hi @Tristan1900. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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-sigs/prow repository.

codecov · 2025-01-15T20:51:54Z

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.6461%. Comparing base (20e6bd2) to head (738a5f3).
Report is 2 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58963        +/-   ##
================================================
+ Coverage   73.0261%   74.6461%   +1.6200%     
================================================
  Files          1684       1700        +16     
  Lines        465659     465784       +125     
================================================
+ Hits         340053     347690      +7637     
+ Misses       104692      96404      -8288     
- Partials      20914      21690       +776

Flag	Coverage Δ
integration	`46.0060% <72.7272%> (?)`
unit	`72.2298% <81.8181%> (+0.0160%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.0354% <86.3636%> (+15.7400%)`	⬆️

3pointer · 2025-01-16T02:36:52Z

br/pkg/restore/log_client/client.go

+
+func PutRawKvWithRetry(ctx context.Context, client *rawkv.RawKVBatchClient, key, value []byte, originTs uint64) error {
+	retry := utils.InitialRetryState(rawKVMaxRetries, rawKVInitialRetryInterval, rawKVMaxRetryInterval)
+	for retry.ShouldRetry() {


you can use utils.WithRetry if you define the InitialRetryState.

Signed-off-by: Wenqi Mou <[email protected]>

BornChanger · 2025-01-17T05:15:35Z

br/pkg/utils/backoff.go

@@ -343,6 +347,7 @@ func NewDiskCheckBackoffStrategy() BackoffStrategy {
 	return NewBackoffStrategy(
 		WithRemainingAttempts(resetTSRetryTime),
 		WithDelayTime(resetTSWaitInterval),
+		WithMaxDelayTime(resetTSMaxWaitInterval),


can those change break the br behavior? my opinion is not to extend this pr's scope but to use another pr to refactor them.

BornChanger · 2025-01-17T05:16:53Z

br/pkg/utils/backoff.go

@@ -353,6 +358,7 @@ func NewRecoveryBackoffStrategy(isRetryErrFunc func(error) bool) BackoffStrategy
 	return NewBackoffStrategy(
 		WithRemainingAttempts(recoveryMaxAttempts),
 		WithDelayTime(recoveryDelayTime),
+		WithMaxDelayTime(recoveryMaxDelayTime),


BornChanger · 2025-01-17T05:16:58Z

br/pkg/utils/backoff.go

@@ -363,6 +369,7 @@ func NewFlashBackBackoffStrategy() BackoffStrategy {
 	return NewBackoffStrategy(
 		WithRemainingAttempts(FlashbackRetryTime),
 		WithDelayTime(FlashbackWaitInterval),
+		WithMaxDelayTime(FlashbackMaxWaitInterval),


BornChanger · 2025-01-17T05:17:04Z

br/pkg/utils/backoff.go

@@ -373,12 +380,24 @@ func NewChecksumBackoffStrategy() BackoffStrategy {
 	return NewBackoffStrategy(
 		WithRemainingAttempts(ChecksumRetryTime),
 		WithDelayTime(ChecksumWaitInterval),
+		WithMaxDelayTime(ChecksumMaxWaitInterval),


BornChanger

rest lgtm.

BornChanger · 2025-01-17T05:17:49Z

/retest

tiprow · 2025-01-17T05:18:11Z

@BornChanger: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

BornChanger · 2025-01-17T07:43:31Z

/retest

tiprow · 2025-01-17T07:43:52Z

@BornChanger: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked do-not-merge/needs-triage-completed and removed do-not-merge/needs-tests-checked labels Jan 15, 2025

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2025

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 15, 2025

3pointer reviewed Jan 16, 2025

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jan 16, 2025

Tristan1900 force-pushed the retry-on-idle-conn branch from 5e698b5 to 563cabd Compare January 17, 2025 02:05

add retry for raw kv client put

362a04f

Signed-off-by: Wenqi Mou <[email protected]>

Tristan1900 force-pushed the retry-on-idle-conn branch from 563cabd to 362a04f Compare January 17, 2025 02:08

fix format

738a5f3

Signed-off-by: Wenqi Mou <[email protected]>

BornChanger reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: add retry for raw kv client put #58963

br: add retry for raw kv client put #58963

Tristan1900 commented Jan 15, 2025 •

edited

Loading

ti-chi-bot bot commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

codecov bot commented Jan 15, 2025 •

edited

Loading

3pointer Jan 16, 2025

BornChanger Jan 17, 2025 •

edited

Loading

BornChanger Jan 17, 2025

BornChanger Jan 17, 2025

BornChanger Jan 17, 2025

BornChanger left a comment

BornChanger commented Jan 17, 2025

tiprow bot commented Jan 17, 2025

BornChanger commented Jan 17, 2025

tiprow bot commented Jan 17, 2025

br: add retry for raw kv client put #58963

Are you sure you want to change the base?

br: add retry for raw kv client put #58963

Conversation

Tristan1900 commented Jan 15, 2025 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

3pointer Jan 16, 2025

Choose a reason for hiding this comment

BornChanger Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

BornChanger Jan 17, 2025

Choose a reason for hiding this comment

BornChanger Jan 17, 2025

Choose a reason for hiding this comment

BornChanger Jan 17, 2025

Choose a reason for hiding this comment

BornChanger left a comment

Choose a reason for hiding this comment

BornChanger commented Jan 17, 2025

tiprow bot commented Jan 17, 2025

BornChanger commented Jan 17, 2025

tiprow bot commented Jan 17, 2025

Tristan1900 commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

BornChanger Jan 17, 2025 •

edited

Loading