-
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
allow users config the count for client report retry dynamicly #783
allow users config the count for client report retry dynamicly #783
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -0,0 +1,932 @@ | |||
package com.alibaba.fescar.rm.datasource; |
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.
copyright
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 add
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 |
ok, I will use client.report.retry.count to this key, and DataProxy will not construct use a map |
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 left one comment
@@ -42,6 +43,8 @@ | |||
|
|||
private ConnectionContext context = new ConnectionContext(); | |||
|
|||
private static int REPORT_RETRY_COUNT = ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_REPORT_RETRY_COUNT, 5); |
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.
5 is magic value , use private static final DEFAULT_RETRY_COUNT
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.
done
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.
done
need final
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
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.
Thanks
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 add client.report.retry.count on file.conf
done |
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 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.
done |
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
@leizhiyuan pr title need modify. |
changed |
@leizhiyuan dingTalk ping me please. |
Ⅰ. 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