Skip to content
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

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

0000005
Copy link
Contributor

@0000005 0000005 commented Nov 30, 2020

Ⅰ. Describe what this PR did

  • Change default config value of CLIENT_TABLE_META_CHECK_ENABLE to true.
  • Make CLIENT_TABLE_META_CHECKER_INTERVAL configurable

Ⅱ. Does this pull request to fix one issue?

fix #3330
Obviously, the current default config value of CLIENT_TABLE_META_CHECK_ENABLE is false.
This default config may cause service unavailable for up to 15 minutes when change table schema.
image
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 to true rather than false.

Ⅲ. 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.
image
And Table mate may not sync immediately. So building afterimage will fail!

Ⅴ. Special notes for reviews

@xingfudeshi
Copy link
Member

Can table_meata_checker_interval be configurable?It may helps for reducing the delay.

@0000005
Copy link
Contributor Author

0000005 commented Dec 3, 2020

Can table_meata_checker_interval be configurable?It may helps for reducing the delay.

ok! I will make it configurable!

@0000005 0000005 changed the title opt: change default config value of CLIENT_TABLE_META_CHECK_ENABLE to true optimize: make CLIENT_TABLE_META_CHECKER_INTERVAL configurable Dec 4, 2020
@0000005
Copy link
Contributor Author

0000005 commented Dec 4, 2020

The work is done, please review my pr.
Travis CI seems to hang for a long time, please help me to trigger it again.
@xingfudeshi

@@ -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;
Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with@l81893521

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@funky-eyes funky-eyes added this to the 1.5.0 milestone Dec 10, 2020
@funky-eyes funky-eyes added the module/rm-datasource rm-datasource module label Dec 10, 2020
@caohdgege
Copy link
Contributor

加了RM的配置,应该还有application.properties以及spring-configuration-metadata.json需要同步修改吧

Copy link
Contributor

@l81893521 l81893521 left a 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.

Copy link
Contributor

@l81893521 l81893521 left a 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.

@l81893521 l81893521 closed this Dec 31, 2020
@l81893521 l81893521 reopened this Dec 31, 2020
@codecov-io
Copy link

codecov-io commented Dec 31, 2020

Codecov Report

Merging #3318 (ee5f099) into develop (5004843) will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            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           
Impacted Files Coverage Δ Complexity Δ
...n/src/main/java/io/seata/common/DefaultValues.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ava/io/seata/core/constants/ConfigurationKeys.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
.../autoconfigure/properties/client/RmProperties.java 45.94% <50.00%> (+0.49%) 8.00 <1.00> (+1.00)
...n/java/io/seata/rm/datasource/DataSourceProxy.java 46.87% <100.00%> (+0.84%) 13.00 <0.00> (ø)

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/rm-datasource rm-datasource module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AT模式下,数据库中表添加一个字段,会导致NullPointerException
7 participants