config: sync store config in time #6919

bufferflies · 2023-08-09T06:11:12Z

What problem does this PR solve?

Issue Number: Close #6918

What is changed and how does it work?

add timeout context for observer tikv config to avoid wait too long

Check List

Tests

Unit test

Code changes

Side effects

Related changes

Need to cherry-pick to the release branch

Release note

sync store config in time

Signed-off-by: bufferflies <[email protected]>

ti-chi-bot · 2023-08-09T06:11:14Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
rleungx

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.

JmPotato

The rest are LGTM.

server/cluster/cluster.go

server/cluster/cluster_test.go

server/cluster/cluster.go

rleungx · 2023-08-09T07:38:00Z

server/cluster/cluster.go

+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, bytes.NewBuffer(nil))
+	if err != nil {
+		cancel()
+		return nil, fmt.Errorf("failed to create store config http request: %w", err)


Do we need to update the store instead of always trying them?

no, it will remove this store if the function return err in line 475.

Will line 475 delete all stores?

only delete the bad store.

But if the PD has a network partition with all TiKV within the sync period, once it recovers, will it have a problem?

I think no, pd will refresh all store state if all the previous stores are bad.

server/cluster/cluster.go

codecov · 2023-08-09T09:31:23Z

Codecov Report

Merging #6919 (3a63f67) into master (68a3e95) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 76.47%.

❗ Current head 3a63f67 differs from pull request most recent head e9dcb0c. Consider uploading reports for the commit e9dcb0c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6919      +/-   ##
==========================================
- Coverage   74.23%   74.22%   -0.01%     
==========================================
  Files         427      427              
  Lines       45149    45163      +14     
==========================================
+ Hits        33517    33524       +7     
- Misses       8667     8675       +8     
+ Partials     2965     2964       -1

Flag	Coverage Δ
unittests	`74.22% <76.47%> (-0.01%)`	⬇️

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

Signed-off-by: bufferflies <[email protected]>

JmPotato · 2023-08-10T03:51:05Z

/merge

ti-chi-bot · 2023-08-10T03:51:07Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2023-08-10T03:51:08Z

This pull request has been accepted and is ready to merge.

Commit hash: b14de0f

ti-chi-bot · 2023-08-10T04:11:08Z

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

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 · 2023-08-10T05:34:04Z

In response to a cherrypick label: new pull request created to branch release-6.1: #6931.

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-08-10T05:34:41Z

In response to a cherrypick label: new pull request created to branch release-6.5: #6932.

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-08-10T05:35:19Z

In response to a cherrypick label: new pull request created to branch release-7.1: #6933.

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

close #6918 add timeout context for observer tikv config to avoid wait too long Signed-off-by: bufferflies <[email protected]>

close #6918 add timeout context for observer tikv config to avoid wait too long Signed-off-by: bufferflies <[email protected]> Co-authored-by: bufferflies <[email protected]> Co-authored-by: buffer <[email protected]>

close #6918 add timeout context for observer tikv config to avoid wait too long Signed-off-by: bufferflies <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: bufferflies <[email protected]>

close tikv#6918 add timeout context for observer tikv config to avoid wait too long Signed-off-by: bufferflies <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

close #6918 add timeout context for observer tikv config to avoid wait too long Co-authored-by: Shirly <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

sync config add context

bea52c9

Signed-off-by: bufferflies <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Aug 9, 2023

ti-chi-bot bot requested review from JmPotato and Yisaer August 9, 2023 06:11

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Aug 9, 2023

bufferflies changed the title ~~sync config add context~~ conifg: sync store config in time Aug 9, 2023

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 Aug 9, 2023

bufferflies requested a review from rleungx August 9, 2023 06:15

JmPotato reviewed Aug 9, 2023

View reviewed changes

server/cluster/cluster.go Outdated Show resolved Hide resolved

server/cluster/cluster_test.go Outdated Show resolved Hide resolved

server/cluster/cluster.go Outdated Show resolved Hide resolved

bufferflies changed the title ~~conifg: sync store config in time~~ config: sync store config in time Aug 9, 2023

bufferflies force-pushed the sync_config branch from a885ede to 959e236 Compare August 9, 2023 07:36

rleungx reviewed Aug 9, 2023

View reviewed changes

JmPotato reviewed Aug 9, 2023

View reviewed changes

server/cluster/cluster.go Outdated Show resolved Hide resolved

use different log

b14de0f

Signed-off-by: bufferflies <[email protected]>

bufferflies force-pushed the sync_config branch from 3a63f67 to b14de0f Compare August 9, 2023 09:41

bufferflies requested review from rleungx and JmPotato August 10, 2023 02:59

rleungx approved these changes Aug 10, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 10, 2023

JmPotato approved these changes Aug 10, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 10, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 10, 2023

ti-chi-bot bot added 2 commits August 10, 2023 03:51

Merge branch 'master' into sync_config

5905bc0

Merge branch 'master' into sync_config

e9dcb0c

ti-chi-bot bot merged commit 38d087f into tikv:master Aug 10, 2023
19 checks passed

ti-chi-bot mentioned this pull request Aug 10, 2023

config: sync store config in time (#6919) #6931

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Aug 10, 2023

This is an automated cherry-pick of tikv#6919

62a8529

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 10, 2023

config: sync store config in time (#6919) #6932

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Aug 10, 2023

This is an automated cherry-pick of tikv#6919

ce7b2df

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 10, 2023

config: sync store config in time (#6919) #6933

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Aug 10, 2023

This is an automated cherry-pick of tikv#6919

b973f44

close tikv#6918 Signed-off-by: ti-chi-bot <[email protected]>

bufferflies mentioned this pull request Aug 11, 2023

config: sync store config in time (#6919) #6931 #6952

Merged

ti-chi-bot bot pushed a commit that referenced this pull request Aug 11, 2023

config: sync store config in time (#6919) #6931 (#6952)

a5c8b89

close #6918 add timeout context for observer tikv config to avoid wait too long Signed-off-by: bufferflies <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: sync store config in time #6919

config: sync store config in time #6919

bufferflies commented Aug 9, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 9, 2023 •

edited

Loading

JmPotato left a comment

rleungx Aug 9, 2023

bufferflies Aug 9, 2023

rleungx Aug 9, 2023

bufferflies Aug 9, 2023

rleungx Aug 9, 2023

bufferflies Aug 9, 2023

rleungx Aug 9, 2023

codecov bot commented Aug 9, 2023 •

edited

Loading

JmPotato commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

config: sync store config in time #6919

config: sync store config in time #6919

Conversation

bufferflies commented Aug 9, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 9, 2023 • edited Loading

JmPotato left a comment

Choose a reason for hiding this comment

rleungx Aug 9, 2023

Choose a reason for hiding this comment

bufferflies Aug 9, 2023

Choose a reason for hiding this comment

rleungx Aug 9, 2023

Choose a reason for hiding this comment

bufferflies Aug 9, 2023

Choose a reason for hiding this comment

rleungx Aug 9, 2023

Choose a reason for hiding this comment

bufferflies Aug 9, 2023

Choose a reason for hiding this comment

rleungx Aug 9, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

JmPotato commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

ti-chi-bot commented Aug 10, 2023

bufferflies commented Aug 9, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading