Added TimeoutPoller to have one thread for timers by shayaantx · Pull Request #842 · microsoft/mssql-jdbc

shayaantx · 2018-10-20T02:58:04Z

As discussed in other pr, modified the TimeoutTimer to not be a unlimited thread pool, instead its one thread that lets TDS commands register/deregister with it, when a timeout is configured, and that thread will timeout the command if necessary. Included test cases.

Updated to include bulk copy functionality.

I literally formatted every single piece of code I touched, it seems much of IO buffer doesn't match the linked formatter, https://github.com/Microsoft/mssql-jdbc/blob/dev/mssql-jdbc_formatter.xml

needs test cases

fix formatting as well

codecov-io · 2018-10-20T03:21:54Z

Codecov Report

Merging #842 into dev will decrease coverage by 0.29%.
The diff coverage is 85.33%.

@@             Coverage Diff             @@
##                dev     #842     +/-   ##
===========================================
- Coverage     48.98%   48.68%   -0.3%     
+ Complexity     2821     2813      -8     
===========================================
  Files           116      118      +2     
  Lines         27877    27863     -14     
  Branches       4650     4637     -13     
===========================================
- Hits          13655    13566     -89     
+ Misses        12104    12101      -3     
- Partials       2118     2196     +78

Flag	Coverage Δ	Complexity Δ
#JDBC42	`48.1% <85.33%> (-0.09%)`	`2759 <14> (+9)`
#JDBC43	`48.66% <85.33%> (-0.26%)`	`2813 <14> (-3)`

Impacted Files	Coverage Δ	Complexity Δ
...a/com/microsoft/sqlserver/jdbc/TimeoutCommand.java	`100% <100%> (ø)`	`5 <5> (?)`
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`55.74% <77.77%> (-2.51%)`	`0 <0> (ø)`
...va/com/microsoft/sqlserver/jdbc/TimeoutPoller.java	`83.78% <83.78%> (ø)`	`9 <9> (?)`
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`52.46% <90%> (-0.27%)`	`263 <0> (ø)`
...m/microsoft/sqlserver/jdbc/SQLServerException.java	`77.44% <0%> (-1.51%)`	`31% <0%> (-2%)`
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	`43.95% <0%> (-1.1%)`	`15% <0%> (ø)`
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	`60.99% <0%> (-1.08%)`	`89% <0%> (-4%)`
...n/java/com/microsoft/sqlserver/jdbc/tdsparser.java	`68.1% <0%> (-0.87%)`	`0% <0%> (ø)`
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	`38.14% <0%> (-0.65%)`	`42% <0%> (-1%)`
... and 7 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 d01e90a...77a2fa3. Read the comment docs.

shayaantx · 2018-10-20T18:06:27Z

@ulvii @cheenamalhotra

shayaantx · 2018-10-21T16:13:00Z

ok sorry, now the formatting should be correct, eclipse kept reverting the formatter I was using on my vm.....

ulvii · 2018-10-26T00:06:37Z

Please add the license header to the new files.

 /*
  * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made
  * available under the terms of the MIT License. See the LICENSE file in the project root for more information.
  */

…and testcase

…/dev

shayaantx · 2018-11-02T02:56:34Z

@ulvii anything other changes required? please let me know

…/dev

…crosoft/dev

… dev

shayaantx · 2018-11-14T00:23:55Z

@ulvii @cheenamalhotra anything I missed?

ulvii · 2018-11-14T00:47:53Z

Hi @shayaantx ,
The review is mostly done and we are targeting to merge the PR for the next preview release. Please stay tuned, as there might still be review comments depending on the results of internal testing.
Thank you for patience.

shayaantx · 2018-11-14T00:49:01Z

@ulvii no problem, just let me know, thanks

peterbae · 2018-11-14T23:01:40Z

Other than that one comment I made, this PR looks good to me.

…ng timeouts and infinite polling checking for timeouts Specifically in microsoft#842, I introduced a single thread to replace the infinite timeout thread pool. However the thread lives forever, so this change seeks to fix that. Also the interrupt logic before/after my change can block timeout threads. So this change also addresses that.

shayaantx added 3 commits October 19, 2018 22:38

Convert TimeoutTimer to single thread

d7d0514

needs test cases

Add docs, add test cases, fix interrupting tds commands

54e76dd

fix formatting as well

Add timeouts to bulk copy command, update testNegativeTimeOut test

1b0171b

Fix formatting

c8531da

rene-ye added this to the 7.1.2 milestone Oct 25, 2018

rene-ye reviewed Oct 25, 2018

View reviewed changes

Comment thread src/main/java/com/microsoft/sqlserver/jdbc/TimeoutCommand.java Outdated