-
Notifications
You must be signed in to change notification settings - Fork 426
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
Support Connection get/setNetworkTimeout(). #253
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #253 +/- ##
============================================
+ Coverage 33.67% 33.71% +0.04%
- Complexity 1490 1498 +8
============================================
Files 98 98
Lines 23422 23447 +25
Branches 3845 3846 +1
============================================
+ Hits 7887 7906 +19
- Misses 13967 13972 +5
- Partials 1568 1569 +1
Continue to review full report at Codecov.
|
* Get the current socket SO_TIMEOUT value. | ||
* | ||
* @return the current socket timeout value | ||
* @throws IOExceptionthrown if the socket timeout cannot be read |
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.
looks like you forgot to put a space between IOException and thrown
} | ||
|
||
public void setNetworkTimeout(Executor executor, | ||
int timeout) throws SQLException { | ||
// this operation is not supported | ||
throw new SQLFeatureNotSupportedException(SQLServerException.getErrString("R_notSupported")); | ||
loggerExternal.entering(getClassNameLogging(), "setNetworkTimeout"); |
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.
we may also want to log the value of timeout by doing something like loggerExternal.entering(getClassNameLogging(), "setNetworkTimeout", timeout);
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.
@brettwooldridge this PR looks perfect to me! Thank you for your contribution. I left a couple of comments here, could you please fix those very minor things before we merge this excellent work?
@v-xiangs Review recommendations integrated. |
@brettwooldridge Thank you for your contribution. |
} | ||
|
||
loggerExternal.exiting(getClassNameLogging(), "getNetworkTimeout"); | ||
return timeout; | ||
} | ||
|
||
public void setNetworkTimeout(Executor executor, |
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.
This misses the requirement to check for permission SQLPermission.setNetworkTimeout
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.
@mrotteveel Thank you for pointing it out. I created a PR #255 for it. Could you or @brettwooldridge review it and see if you are satisfied with it? Thank you.
This is essential to minimize the effects of network partition events.
Related to #72 and #85.