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

Convert socket adaptor to socket #160

Merged

Conversation

xiangyushawn
Copy link
Contributor

for issue #144

@codecov-io
Copy link

Codecov Report

Merging #160 into dev will decrease coverage by -0.13%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##                dev     #160      +/-   ##
============================================
- Coverage     29.77%   29.65%   -0.13%     
+ Complexity     1251     1247       -4     
============================================
  Files            97       97              
  Lines         23303    23305       +2     
  Branches       3871     3871              
============================================
- Hits           6939     6910      -29     
- Misses        15014    15043      +29     
- Partials       1350     1352       +2
Flag Coverage Δ Complexity Δ
#JDBC41 29.58% <0%> (-0.03%) 1244 <0> (-3)
#JDBC42 29.57% <0%> (-0.14%) 1245 <0> (-2)
Impacted Files Coverage Δ Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 32% <0%> (-0.69%) 0 <0> (ø)
src/main/java/microsoft/sql/DateTimeOffset.java 37.14% <0%> (-2.86%) 8% <0%> (-2%)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 38.24% <0%> (-0.72%) 50% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 45.67% <0%> (-0.37%) 182% <0%> (-1%)

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 6e8dc59...c086691. Read the comment docs.

@Suraiya-Hameed Suraiya-Hameed merged commit 369e5e0 into microsoft:dev Feb 28, 2017
@xiangyushawn xiangyushawn deleted the convert-SocketAdaptor-to-Socket branch February 28, 2017 00:48
@sehrope
Copy link
Contributor

sehrope commented Feb 28, 2017

I tried out our test case and it fixes the cancellation issue when multiSubnetFailover is enabled. It no longer hangs and the cancellation happens correctly. Very nice.

Skimming through the implementation, it looks like the approach is to close the socket channel and reconnect it as a regular socket. That should be be fine though it does lead to the situation that all multi subnet enabled connections will create two socket connections to the server (the first being closed). It'd be marginally slower than directly using the channel but it shouldn't be a big deal as it's just the establishment of the TCP socket. Also, I don't think it's an issue on the server to have the first connection closed as the driver is already doing that to the rest of the resolved addresses anyway.

So looks good to me.

FYI, the commit message has a typo: Convert socket adaptor to socket #160. Adaptor should be spelled adapter). Looks like it's already been merged though ...

@xiangyushawn
Copy link
Contributor Author

@sehrope Thank you for getting back to us. We are glad to know you are satisfied with this fix.
Forgive me about the typo. English isn't my first language, Java is :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants