-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: adjust delta schema count and add metrics #11625
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11625 +/- ##
===============================================
- Coverage 79.7436% 79.671% -0.0726%
===============================================
Files 462 461 -1
Lines 102106 101461 -645
===============================================
- Hits 81423 80835 -588
- Misses 14799 14810 +11
+ Partials 5884 5816 -68 |
@zimulala Please fix ci |
PTAL @tiancaiamao @jackysp @winkyao |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @jackysp. |
domain/schema_validator.go
Outdated
maxCnt := int(variable.GetMaxDetalSchemaCount()) | ||
if len(s.deltaSchemaInfos) > maxCnt && s.notMergeCnt > maxCnt/2 { | ||
s.merge() | ||
s.notMergeCnt = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set to 1 here? I think we don't know how many deltaSchemaInfos
merged here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call merge
before setting the value. So I think that the previous deltaSchemaInfos
have been merged.
PTAL @jackysp @tiancaiamao @winkyao |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PTAL @tiancaiamao @winkyao |
PTAL @bb7133 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
PTAL @AilinKid @tiancaiamao |
s.deltaSchemaInfos = s.deltaSchemaInfos[1:] | ||
} | ||
} | ||
|
||
type ids []int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defined but not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, used in line 248
LGTM |
/run-all-tests |
LGTM |
cherry pick to release-2.1 failed |
cherry pick to release-3.0 failed |
What problem does this PR solve?
The original size of
deltaSchemaInfos
is 100.Executing some SQLs may take a long time. At the same time, the TiDB cluster may do a lot of DDLs, which means we change a lot of schema versions. And the schema version changed in it may exceed 100. Then the SQLs will return the error of
Information schema is changed.
.If the DDL owner TiDB and the receiving request TiDB are on the same TiDB, it takes 50 + ms to update each mode version.
If the DDL owner TiDB and the receiving request TiDB aren't on the same TiDB, it takes 500 + ms to update each mode version.
The default of
MaxTxnTimeUse
is 590s. So I set the size ofdeltaSchemaInfos
to 1024.What is changed and how it works?
Add a variable of
TiDBMaxDeltaSchemaCount
to update the max size ofdeltaSchemaInfos
. Besides, add a function ofmerge
Add some metrics.
Check List
Tests
Related changes