-
Notifications
You must be signed in to change notification settings - Fork 8.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
optimize: make CLIENT_TABLE_META_CHECKER_INTERVAL configurable #3318
Conversation
Can table_meata_checker_interval be configurable?It may helps for reducing the delay. |
ok! I will make it configurable! |
The work is done, please review my pr. |
@@ -30,7 +30,8 @@ | |||
int DEFAULT_TM_DEGRADE_CHECK_PERIOD = 2000; | |||
int DEFAULT_CLIENT_REPORT_RETRY_COUNT = 5; | |||
boolean DEFAULT_CLIENT_REPORT_SUCCESS_ENABLE = false; | |||
boolean DEFAULT_CLIENT_TABLE_META_CHECK_ENABLE = false; | |||
boolean DEFAULT_CLIENT_TABLE_META_CHECK_ENABLE = true; |
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.
I think the configuration of DEFAULT_CLIENT_TABLE_META_CHECK_ENABLE keep false in default
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.
I agree with@l81893521
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?
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.
In a production environment, the database table structure generally does not change frequently.
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.
if it does change frequently,then the checking should be enabled manually and the interval of checking should be set a fit value too.That's why making interval to be configurable is enough.
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.
I agree to keep DEFAULT_CLIENT_TABLE_META_CHECK_ENABLE default value as false.
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.
ok, I will remove this piece of code when I have time.
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.
The work is done, please review my pr. @xiajunhust @l81893521 @slievrly
加了RM的配置,应该还有application.properties以及spring-configuration-metadata.json需要同步修改吧 |
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.
Please solve the conflict.
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.
And please record in /changes module.
Codecov Report
@@ Coverage Diff @@
## develop #3318 +/- ##
==========================================
Coverage 51.57% 51.57%
- Complexity 3373 3374 +1
==========================================
Files 617 617
Lines 20455 20460 +5
Branches 2565 2565
==========================================
+ Hits 10550 10553 +3
- Misses 8844 8846 +2
Partials 1061 1061
|
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.
LGTM
Ⅰ. Describe what this PR did
Change default config value of CLIENT_TABLE_META_CHECK_ENABLE totrue
.Ⅱ. Does this pull request to fix one issue?
fix #3330
Obviously, the current default config value of
CLIENT_TABLE_META_CHECK_ENABLE
isfalse
.This default config may cause service unavailable for up to 15 minutes when change table schema.
For example, add a new column to a table. This is a very common operation when you update your system.
I think that 15 minutes unavailable is an unacceptable duration for updating a system.
So, It is better to change the default config of
CLIENT_TABLE_META_CHECK_ENABLE
totrue
rather thanfalse
.Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
step1: add a new column to a table.
step2: Execute insert SQL by seata. The insert table must be the table that we changed in step1.
stop3: Insert will not succeed!
Why is that?
Because seata need to build afterImage after the real insert is executed.
It will select all columns When building afterImage.
And Table mate may not sync immediately. So building afterimage will fail!
Ⅴ. Special notes for reviews