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.

Sign up for GitHub

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

Jump to bottom

allow users config the count for client report retry dynamicly #783

Merged
merged 9 commits into from
Apr 15, 2019
Merged

allow users config the count for client report retry dynamicly #783

merged 9 commits into from
Apr 15, 2019

Conversation

leizhiyuan
Copy link
Contributor

Ⅰ. Describe what this PR did

allow user set the parameters of proxy

Ⅱ. Does this pull request fix one issue?

fixed #782

Ⅲ. Why don't you add test cases (unit test/integration test)?

yes

Ⅳ. Describe how to verify it

test case

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Apr 13, 2019 •
edited

Codecov Report

Merging #783 into develop will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #783      +/-   ##
=============================================
+ Coverage      34.22%   34.22%   +<.01%     
  Complexity       887      887              
=============================================
  Files            215      215              
  Lines           8299     8300       +1     
  Branches         996      996              
=============================================
+ Hits            2840     2841       +1     
  Misses          5119     5119              
  Partials         340      340
Impacted Files Coverage Δ Complexity Δ
...ibaba/fescar/core/constants/ConfigurationKeys.java 0% <ø> (ø) 0 <0> (ø) ⬇️
.../alibaba/fescar/rm/datasource/ConnectionProxy.java 9.52% <50%> (+1.09%) 4 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2a695...2016516. Read the comment docs.

slievrly
slievrly reviewed Apr 14, 2019
View reviewed changes
rm-datasource/src/test/java/com/alibaba/fescar/rm/datasource/DataSourceProxyTest.java Outdated
@@ -0,0 +1,932 @@
package com.alibaba.fescar.rm.datasource;
Copy link
Member

Choose a reason for hiding this comment

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

copyright

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 add

@CoffeeLatte007
Copy link
Contributor

Can u use Config Instead of customized a map?

@leizhiyuan
Copy link
Contributor Author

Can u use Config Instead of customized a map?

what you mean is to create a dataproxyconfig class to store this config explicitly, right?

@CoffeeLatte007
Copy link
Contributor

Can u use Config Instead of customized a map?

what you mean is to create a dataproxyconfig class to store this config explicitly, right?

you can see
LockRetryController private static int LOCK_RETRY_TIMES = ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_TIMES, 30);

@leizhiyuan
Copy link
Contributor Author

LockRetryController

ok, I will use client.report.retry.count to this key, and DataProxy will not construct use a map

CoffeeLatte007
CoffeeLatte007 reviewed Apr 15, 2019
View reviewed changes
Copy link
Contributor

@CoffeeLatte007 CoffeeLatte007 left a comment

Choose a reason for hiding this comment

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

I left one comment

rm-datasource/src/main/java/com/alibaba/fescar/rm/datasource/ConnectionProxy.java Outdated
@@ -42,6 +43,8 @@

private ConnectionContext context = new ConnectionContext();

private static int REPORT_RETRY_COUNT = ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_REPORT_RETRY_COUNT, 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

5 is magic value , use private static final DEFAULT_RETRY_COUNT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

done

need final

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

CoffeeLatte007
CoffeeLatte007 approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@CoffeeLatte007 CoffeeLatte007 left a comment

Choose a reason for hiding this comment

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

Thanks

slievrly
slievrly reviewed Apr 15, 2019
View reviewed changes
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

please add client.report.retry.count on file.conf

@leizhiyuan
Copy link
Contributor Author

file.conf

done

slievrly
slievrly reviewed Apr 15, 2019
View reviewed changes
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

If you use the file for configuration, there are 3 files.conf that need to be modified. If you use a third-party configuration center, you only need to modify the configuration center configuration.

@leizhiyuan
Copy link
Contributor Author

done

slievrly
slievrly approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly
Copy link
Member

@leizhiyuan pr title need modify.

@leizhiyuan leizhiyuan changed the title allow user set the parameters of proxy allow user set the count for client report retry Apr 15, 2019
@leizhiyuan leizhiyuan changed the title allow user set the count for client report retry allow users config the count for client report retry dynamicly Apr 15, 2019
@leizhiyuan
Copy link
Contributor Author

changed

@slievrly slievrly merged commit 18e53c3 into apache:develop Apr 15, 2019
@slievrly
Copy link
Member

@leizhiyuan dingTalk ping me please.

@fescar-robot fescar-robot mentioned this pull request Apr 19, 2019
Closed
@wangliang181230 wangliang181230 added this to the 0.5.* milestone Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slievrly slievrly slievrly approved these changes

@CoffeeLatte007 CoffeeLatte007 CoffeeLatte007 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.*
Development

Successfully merging this pull request may close these issues.

Configure branchReport retry times
5 participants
@leizhiyuan @codecov-io @CoffeeLatte007 @slievrly @wangliang181230