ddl: refactor constraint [2/6] #24005

xhebox · 2021-04-14T06:16:22Z

What problem does this PR solve?

Problem Summary: Separate from #22415, 2/6 patch from the large PR.

Able to review, but merge depends on #24004

LabelConstraint is renamed to Constraint.
checkLabelConstraint to NewConstraint
added CompatibleWith, which is used to detect invalid combination of constraint. It is not used in the PR, but in the next PR.
Move all Constraint things to constraint.go, and a full cover test in contraint_test.go.

Check List

Tests

Unit test
Integration test

Release note

No release note

morgo

Looking good, minor nits.

ddl/placement/constraint_test.go

morgo · 2021-04-14T14:20:18Z

ddl/placement/constraint_test.go

+			name:  "not tiflash",
+			input: "-engine  =  tiflash  ",
+			label: Constraint{
+				Key:    "engine",
+				Op:     NotIn,
+				Values: []string{"tiflash"},
+			},


Should it have some tests where the case of tiflash is upper? The code lowers it for comparison, but does not save it in values as lower as it parses it (making it compatible with previous code). But I'm wondering if it should lower when saving in values so the string is effectively normalized (space removed, consistent case etc.)

I think there is one test for it, the typo one pointed out by you.

{ name: "disallow tiflash", input: "+engine=Tiflash", err: ErrUnsupportedConstraint, },

But I'm wondering if it should lower when saving in values so the string is effectively normalized.

There should not be any normalization during comparison. I mean, I only do lowering for TiFlash, and it is a special case. It seems that we have no official doc, or spec for upper/lower cases of engine labels, so i added lower checking for a more relax check(so that tiflash will be banned even if it is TiFlash, Tiflash, or whatever).

It is a bit of a strange behavior if the token TiFlash is case insensitive, but other tokens are stored in original case?

I think tokens are always case insensitive in MySQL, but in either case you decide (or don't decide), I think it makes sense to capture the current expected behavior in tests:

input: "+ZONE=BJ", input: "+Zone=Bj",

The thing is it is not a token, but a string. The syntax is like CONSTRIANTS="+zone=bj". Here we use this string a "pattern matcher", which will match constraints added by user, or the software itself. PD and TiDB both have a matcher: TiDB will match on the constraints cache, PD will match on the persisted constraints.

So, there are other two concerns:

zone,bj kv pair is added by a user. But possibly not by a SQL of TiDB, e.g. tidb-operator could add such label. They won't normalize the label.

PD match case insensitively. It is also strange that TiDB is insensitive, but PD is sensitive.

For TiFlash, it is just, I don't know, if the value is always TiFlash. I mean, there is no spec for that. And this label is absolutely added by TiKV,TiFlash,TiDB. User should not add engine=xxx label. It will break the system.

The thing is it is not a token, but a string. The syntax is like ALTER TABLE ALTER PARTITION ADD PLACEMENT POLICY ... CONSTRIANTS="+zone=bj,..." .... I've proposed to parse the string by parser, but rejected by djshow832. It indeed makes the work easier, because this string may contain comma.

Here we use this string a "pattern matcher", which will match constraints added by user, or the software itself. PD and TiDB both have a matcher: TiDB will match on the constraints cache, PD will match on the persisted constraints.

So, there are other two concerns:

zone,bj kv pair is added by a user. But possibly not by a SQL of TiDB, e.g. tidb-operator could add such label. They won't normalize the label.

PD match case sensitively. It is also strange that TiDB is insensitive, but PD is sensitive.

For TiFlash, it is just, I don't know, if the value is always TiFlash. I mean, there is no spec for that. And this label is absolutely added by TiKV,TiFlash,TiDB. User should not add engine=xxx label. It will break the system.

OK. It seems complicated enough maybe we can't solve it in this PR. LGTM.

go.sum

ddl/placement/types.go

morgo · 2021-04-15T00:58:05Z

/LGTM

xhebox · 2021-04-15T01:10:59Z

Able to review, but merge depends on #24004
/hold

xhebox · 2021-04-16T02:36:19Z

/unhold

Signed-off-by: xhe <xw897002528@gmail.com>

Co-authored-by: Morgan Tocker <tocker@gmail.com>

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox · 2021-04-16T05:25:38Z

Rebased to the master, because the first PR is merged.

ddl/placement/constraint.go

ddl/placement/constraint_test.go

tangenta · 2021-04-16T09:54:48Z

ddl/placement/constraint.go

+func (c *Constraint) String() string {
+	str, err := c.Restore()
+	if err != nil {
+		return err.Error()


The String() returning error message seems to be strange...

Does panic feel better?

How about log it and return empty("")?

That is too silent... The only way to construct invalid constraint is by manual construction, bypassing the new method and all other checks. So far, only tests and json.Unmarshal does not use the new method(after the merge of all refactor PRs). Unmarshal takes data from the data persisted on PD, which are constructed through SQL and the new method. So it is safe, too.

User just should not go here. And devs should not rely on manual manipulation if there is a set of working public APIs.

EDIT: It seems taht only tests are using String() for debugging purpose.

If it is not used in the normal case, we should not provide it. 🤔

But to use json.Marshal convert the struct to string, there must be one stringer implementation.

Ok... Anyway, Golang cannot prohibit the developers from initializing a struct with manual construction. Panic can be dangerous in a production environment. I still prefer logging it and return an empty string:)

We can prevent that by wrapping another internal package and private fields... Manual construction will be blocked unless you are in the placement package. E.g.:

import "mypackage/internal" type MyStruct struct { internalFields internal.MyRealStruct }

But that seems an overkill for me. It also changes the structure of json. So it can not be marshaled easily.

Anyway, Golang cannot prohibit the developers from initializing a struct with manual construction.

In fact, one of my point for refactor is to prevent developers from manual/raw operations. I hope they can follow the contract before going deep into the package.

Panic can be dangerous in a production environment

The upper service should recover() the error, if there is a need.

Anyway, I removed the stringer interface.

Co-authored-by: tangenta <tangenta@126.com>

Signed-off-by: xhe <xw897002528@gmail.com>

tangenta · 2021-04-16T16:39:19Z

/lgtm

ti-chi-bot · 2021-04-16T16:39:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo
tangenta

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

tangenta · 2021-04-16T16:39:49Z

/merge

ti-chi-bot · 2021-04-16T16:39:52Z

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

Commit hash: 3c8921f

ti-chi-bot · 2021-04-16T16:39:59Z

@xhebox: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

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.

xhebox requested a review from a team as a code owner April 14, 2021 06:16

xhebox requested review from wshwsh12 and removed request for a team April 14, 2021 06:16

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 14, 2021

xhebox changed the title ~~ddl: refactor constraint~~ ddl: refactor constraint [2/6] Apr 14, 2021

github-actions bot added component/expression sig/sql-infra SIG: SQL Infra labels Apr 14, 2021

xhebox requested a review from a team April 14, 2021 07:28

morgo reviewed Apr 14, 2021

View reviewed changes

ddl/placement/types.go Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 15, 2021

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2021

xhebox and others added 4 commits April 16, 2021 13:23

ddl: refactor constraint

e40bc22

Signed-off-by: xhe <xw897002528@gmail.com>

ddl: typo

f6a7d80

Co-authored-by: Morgan Tocker <tocker@gmail.com>

ddl: fmt

76b1607

Signed-off-by: xhe <xw897002528@gmail.com>

*: revert go mod

0c0a7cf

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox force-pushed the master_2 branch from d0aa0e7 to 0c0a7cf Compare April 16, 2021 05:23

tangenta reviewed Apr 16, 2021

View reviewed changes

xhebox and others added 3 commits April 16, 2021 18:06

ddl: address comments

18b5771

Co-authored-by: tangenta <tangenta@126.com>

ddl: panic if the input string is invalid

eff1f9b

Signed-off-by: xhe <xw897002528@gmail.com>

ddl: remove stringer

3c8921f

Signed-off-by: xhe <xw897002528@gmail.com>

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 16, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 16, 2021

Merge branch 'master' into master_2

b3f9820

ti-chi-bot merged commit 3ec3729 into pingcap:master Apr 16, 2021

xhebox deleted the master_2 branch April 20, 2021 01:10

xhebox mentioned this pull request Jul 7, 2021

Define the placement of data by SQL statements #18030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: refactor constraint [2/6] #24005

ddl: refactor constraint [2/6] #24005

xhebox commented Apr 14, 2021 •

edited

Loading

morgo left a comment

morgo Apr 14, 2021

xhebox Apr 14, 2021 •

edited

Loading

morgo Apr 14, 2021

xhebox Apr 15, 2021

xhebox Apr 15, 2021 •

edited

Loading

morgo Apr 15, 2021

morgo commented Apr 15, 2021

xhebox commented Apr 15, 2021 •

edited

Loading

xhebox commented Apr 16, 2021

xhebox commented Apr 16, 2021

tangenta Apr 16, 2021

xhebox Apr 16, 2021

tangenta Apr 16, 2021

xhebox Apr 16, 2021 •

edited

Loading

tangenta Apr 16, 2021

xhebox Apr 16, 2021

tangenta Apr 16, 2021 •

edited

Loading

xhebox Apr 16, 2021 •

edited

Loading

xhebox Apr 16, 2021

tangenta commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

tangenta commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

ddl: refactor constraint [2/6] #24005

ddl: refactor constraint [2/6] #24005

Conversation

xhebox commented Apr 14, 2021 • edited Loading

What problem does this PR solve?

Check List

Release note

morgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhebox Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhebox Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgo commented Apr 15, 2021

xhebox commented Apr 15, 2021 • edited Loading

xhebox commented Apr 16, 2021

xhebox commented Apr 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhebox Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

xhebox Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

tangenta commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

xhebox commented Apr 14, 2021 •

edited

Loading

xhebox Apr 14, 2021 •

edited

Loading

xhebox Apr 15, 2021 •

edited

Loading

xhebox commented Apr 15, 2021 •

edited

Loading

xhebox Apr 16, 2021 •

edited

Loading

tangenta Apr 16, 2021 •

edited

Loading

xhebox Apr 16, 2021 •

edited

Loading