-
Notifications
You must be signed in to change notification settings - Fork 26.5k
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
Cleanup and throw exception if bind failed #12987
Conversation
Codecov Report
@@ Coverage Diff @@
## 3.2 #12987 +/- ##
=============================================
+ Coverage 68.78% 69.45% +0.67%
+ Complexity 341 2 -339
=============================================
Files 3659 1649 -2010
Lines 171227 68513 -102714
Branches 28256 10001 -18255
=============================================
- Hits 117770 47589 -70181
+ Misses 43281 16330 -26951
+ Partials 10176 4594 -5582 see 2048 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/server/Server.java
Outdated
Show resolved
Hide resolved
Throwable lastError = null; | ||
int retryTimes = getUrl().getParameter(Constants.BIND_RETRY_TIMES, 10); | ||
int retryInterval = getUrl().getParameter(Constants.BIND_RETRY_INTERVAL, 3000); | ||
for (int i = 0; i < retryTimes; i++) { |
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 would we need binding retry, what's the problem with throwing exception directly on binding failure.
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.
Such 20880, 22222... are in ip_local_port_range
and can be used as a tcp client port. If there is someone requesting remoting using 20880 as client port(allocated by kernel), Dubbo will bind failed.
if (qosCheck) { | ||
throw new IllegalStateException("Fail to start qos server: " + throwable.getMessage(), throwable); | ||
throw new RpcException(throwable); |
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.
Unnecessary wrap, how about throw it directly.
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.
Kudos, SonarCloud Quality Gate passed! |
What is the purpose of the change
Brief changelog
Verifying this change
Checklist