-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
binlog: update config for backward compatibility #9688
binlog: update config for backward compatibility #9688
Conversation
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
Please fix CI. |
This looks like a great solution. I have one concern: the new variable is a string with one allowed value besides empty: "auto". Can we validate this so that we reject other values? We could instead make the "mode" a boolean so that it will be validated, but these changes show that a string is more extensible for future changes. |
@gregwebs I changed |
Codecov Report
@@ Coverage Diff @@
## master #9688 +/- ##
================================================
+ Coverage 67.3493% 67.3627% +0.0134%
================================================
Files 378 378
Lines 79542 79544 +2
================================================
+ Hits 53571 53583 +12
+ Misses 21193 21185 -8
+ Partials 4778 4776 -2 |
/run-all-tests |
LGTM |
/run-all-tests |
2 similar comments
/run-all-tests |
/run-all-tests |
/run-common-test |
1 similar comment
/run-common-test |
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
/run-all-tests |
/run-common-test tidb-test=pr/739 |
This reverts commit 792429d.
What problem does this PR solve?
pr #9625 break the backward compatibility,
this pr fix it
What is changed and how it works?
Check List
Tests
Related changes