Use Socket instead of SocketChannel when multiSubnetFailover=true

[ulvii]

Fix for both #144 and #645 .This PR reverts #160.

Switching from SocketChannels to Sockets to avoid creating 2 connections when multiSubnetFailover=true.

[codecov-io]

Codecov Report

Merging #662 into dev will increase coverage by 0.28%.
The diff coverage is 62.5%.

@@             Coverage Diff              @@
##                dev     #662      +/-   ##
============================================
+ Coverage     48.08%   48.37%   +0.28%     
- Complexity     2575     2593      +18     
============================================
  Files           113      113              
  Lines         26574    26574              
  Branches       4429     4430       +1     
============================================
+ Hits          12778    12854      +76     
- Misses        11665    11674       +9     
+ Partials       2131     2046      -85

Flag	Coverage Δ	Complexity Δ
#JDBC42	48.24% <62.5%> (+0.27%)	2589 <0> (+19)	⬆️
#JDBC43	47.92% <62.5%> (-0.1%)	2563 <0> (-9)

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	56.42% <62.5%> (+2.29%)	0 <0> (ø)	⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	44.74% <0%> (-0.45%)	106% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	32.73% <0%> (-0.29%)	241% <0%> (-3%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	52.54% <0%> (-0.13%)	239% <0%> (-2%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	63.05% <0%> (-0.13%)	0% <0%> (ø)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	44.94% <0%> (ø)	16% <0%> (ø)	⬇️
...m/microsoft/sqlserver/jdbc/SQLServerException.java	77.23% <0%> (ø)	31% <0%> (+1%)	⬆️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	61.57% <0%> (ø)	91% <0%> (+2%)	⬆️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	38.75% <0%> (+0.21%)	42% <0%> (ø)	⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java	59.81% <0%> (+0.3%)	137% <0%> (+6%)	⬆️
... and 3 more

---

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 3a8c146...a821e1f. Read the comment docs.

[ulvii]

Hi @greg-pendlebury and @sehrope,

We would very much appreciate if you could test the attached jars against #144 and #645.

[greg-pendlebury]

Thanks @ulvii, one of our DBAs tested the jar with JMeter using the same test we used in demonstrating #645. He has confirmed that the NUMA affinity issues are resolved. We will try updating our full application once a release of the driver is available, but the regression testing of all the other changes since 6.1.7 might take us some time.

[sehrope]

Sure we'll get this tested. Are the jars you've attached built off the tip of this PR? i.e. can I just build the PR?

I took a peek at the diff itself and it's quite minimal. The main change seems to be replacing findSocketUsingJavaNIO(...) with findSocketUsingThreading(...) which has existed at least since the open sourcing of the driver but doesn't seem to have been referenced in any active code.

[ulvii]

Thanks @greg-pendlebury , @sehrope .

@sehrope , yes the jars are built on top of the PR.

[sehrope]

We tested this a bit and it doesn't break query cancellation when multiSubnetFailover is enabled.

I think it'd be worthwhile to eventually rewrite how SocketFinder is implemented. The concept itself is fine, i.e use a thread pool to race opening sockets to all resolved addresses. The complexity is that it's done using manual locking, .wait(...), .notify() and the code is intermixed with the parent class. It likely predates modern concurrency primitives and redoing it using something like Future or CountDownLatch would simplify things.

The current code also handles IPv4 and IPv6 sequentially. I don't see that behavior explicitly documented anywhere nor does it seem like it'd be useful. If a user has multiple address types resolving from a host name they should all be attempted in parallel right? I took a stab at a high level clean up for this. See here for a PR: #663