mcs: watch rule change with txn #7550

lhy1024 · 2023-12-14T14:52:57Z

What problem does this PR solve?

Issue Number: Close #7418

What is changed and how does it work?

Check List

Tests

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

Release note

None.

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2023-12-14T14:53:01Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
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.

ti-chi-bot · 2023-12-14T14:53:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-12-14T15:05:17Z

Codecov Report

Merging #7550 (fc61c05) into master (59c9d04) will decrease coverage by 0.08%.
The diff coverage is 64.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7550      +/-   ##
==========================================
- Coverage   74.80%   74.73%   -0.08%     
==========================================
  Files         457      457              
  Lines       50351    50388      +37     
==========================================
- Hits        37667    37658       -9     
- Misses       9351     9396      +45     
- Partials     3333     3334       +1

Flag	Coverage Δ
unittests	`74.73% <64.21%> (-0.08%)`	⬇️

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

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-12-18T03:40:46Z

pkg/mcs/scheduling/server/rule/watcher.go

+			}
+			// Try to add the rule to the patch or directly update the rule manager.
+			err = func() error {
+				if rw.patch == nil {


When will this happen?

In this pr, it will not use patch when loading data.

Maybe we can only use patch in the all time

tests/server/api/region_test.go

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx

The rest LGTM

pkg/mcs/scheduling/server/rule/watcher.go

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-12-19T04:05:14Z

pkg/mcs/scheduling/server/rule/watcher.go

-		rule, err := placement.NewRuleFromJSON([]byte(ruleJSON))
-		if err != nil {
+	}
+	postFn := func(events []*clientv3.Event) error {


The postFn may not be executed when calling load.

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-12-19T04:23:42Z

pkg/utils/etcdutil/etcdutil.go

+		log.Error("run pre event failed in watch loop", zap.String("name", lw.name),
+			zap.String("key", lw.key), zap.Error(err))
+	}
+	defer func() {


Will it affect the keyspace related logic?

I will check it

This PR will also call postEventsFn when meeting failures in loading data.

After my check, keyspace-watcher will call postEventsFn to retry, keyspace-server-watcher will call postEventsFn to send keyspaces.

So I modify a little about it.

The return error also changes, right?

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2023-12-19T05:07:09Z

These tests will be failed with master branch in api mode

Signed-off-by: lhy1024 <admin@liudos.us>

CabinfeverB · 2023-12-19T06:49:42Z

pkg/mcs/scheduling/server/rule/watcher.go

+				return err
+			}
+			// Try to add the rule change to the patch.
+			if err := rw.ruleManager.AdjustRule(rule, ""); err != nil {


Do we need to parser the group ID and use it for this function?

This pr has used rule.GroupID in value

CabinfeverB · 2023-12-19T07:08:24Z

pkg/utils/etcdutil/etcdutil.go

@@ -865,6 +865,16 @@ func (lw *LoopWatcher) load(ctx context.Context) (nextRevision int64, err error)
 	if limit != 0 {
 		limit++
 	}
+	if err := lw.preEventsFn([]*clientv3.Event{}); err != nil {


It seems that there is no implemented function using the param.

It is used to judge whether is called by watch or by load.

CabinfeverB · 2023-12-19T08:47:16Z

pkg/mcs/scheduling/server/rule/watcher.go

+			if isLoading && rw.patch.IsRuleExist(rule) {
+				log.Error("duplicated rule key", zap.String("rule-key", key), zap.ByteString("rule-value", kv.Value),
+					errs.ZapError(errs.ErrLoadRule))
+				return errs.ErrLoadRule


If we return error, the cluster can not be created. Is it expected?

It should be deleted I think.

rleungx · 2023-12-19T09:11:49Z

pkg/mcs/scheduling/server/rule/watcher.go

@@ -129,6 +132,11 @@ func (rw *Watcher) initializeRuleWatcher() error {
 			if err := rw.ruleManager.AdjustRule(rule, ""); err != nil {
 				return err
 			}
+			if isLoading && rw.patch.IsRuleExist(rule) {


Do we need to check the store key?

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2023-12-20T03:48:26Z

/merge

ti-chi-bot · 2023-12-20T03:48:27Z

@lhy1024: 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-12-20T03:48:29Z

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

Commit hash: acacc02

ti-chi-bot · 2023-12-20T03:48:42Z

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

mcs: watch rule change with txn

69e2e90

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 14, 2023

ti-chi-bot bot requested review from JmPotato and rleungx December 14, 2023 14:53

refactor endpoint

2b1c62c

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Dec 18, 2023

View reviewed changes

rleungx reviewed Dec 19, 2023

View reviewed changes

tests/server/api/region_test.go Outdated Show resolved Hide resolved

lhy1024 added 2 commits December 19, 2023 11:21

Merge branch 'master' of github.com:tikv/pd into fix-rule-sync3

2e1d11b

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'master' of github.com:tikv/pd into fix-rule-sync3

9c0a7f2

lhy1024 force-pushed the fix-rule-sync3 branch from f3123be to 9c0a7f2 Compare December 19, 2023 03:24

rleungx requested a review from CabinfeverB December 19, 2023 03:44

rleungx reviewed Dec 19, 2023

View reviewed changes

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

pkg/mcs/scheduling/server/rule/watcher.go Outdated Show resolved Hide resolved

address comments and remove nopatch

1bd2f1d

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Dec 19, 2023

View reviewed changes

address comments and move post function

1e778c5

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Dec 19, 2023

View reviewed changes

rleungx marked this pull request as ready for review December 19, 2023 04:23

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 Dec 19, 2023

add test

d4046e1

Signed-off-by: lhy1024 <admin@liudos.us>

address comments

e103130

Signed-off-by: lhy1024 <admin@liudos.us>

CabinfeverB reviewed Dec 19, 2023

View reviewed changes

CabinfeverB approved these changes Dec 19, 2023

View reviewed changes

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

CabinfeverB reviewed Dec 19, 2023

View reviewed changes

rleungx reviewed Dec 19, 2023

View reviewed changes

lhy1024 force-pushed the fix-rule-sync3 branch from 5d9cfae to 969b737 Compare December 19, 2023 11:06

lhy1024 added 2 commits December 19, 2023 19:06

add loading log

969b737

Signed-off-by: lhy1024 <admin@liudos.us>

Merge branch 'master' into fix-rule-sync3

acacc02

rleungx approved these changes Dec 20, 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 Dec 20, 2023

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

Merge branch 'master' into fix-rule-sync3

fc61c05

ti-chi-bot bot merged commit cfbc9b9 into tikv:master Dec 20, 2023
24 of 26 checks passed

lhy1024 deleted the fix-rule-sync3 branch December 20, 2023 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: watch rule change with txn #7550

mcs: watch rule change with txn #7550

lhy1024 commented Dec 14, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 14, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 14, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

rleungx Dec 18, 2023

lhy1024 Dec 19, 2023

lhy1024 Dec 19, 2023

rleungx left a comment

rleungx Dec 19, 2023

rleungx Dec 19, 2023

lhy1024 Dec 19, 2023

lhy1024 Dec 19, 2023

rleungx Dec 19, 2023

lhy1024 commented Dec 19, 2023

CabinfeverB Dec 19, 2023

lhy1024 Dec 19, 2023

CabinfeverB Dec 19, 2023

lhy1024 Dec 19, 2023

CabinfeverB Dec 19, 2023

rleungx Dec 19, 2023

rleungx Dec 19, 2023

lhy1024 commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

mcs: watch rule change with txn #7550

mcs: watch rule change with txn #7550

Conversation

lhy1024 commented Dec 14, 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 Dec 14, 2023 • edited Loading

ti-chi-bot bot commented Dec 14, 2023

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a 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

lhy1024 commented Dec 19, 2023

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

lhy1024 commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

lhy1024 commented Dec 14, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading