Executor service instead of thread #162

xiangyushawn · 2017-02-28T00:52:43Z

for issue #150

codecov-io · 2017-02-28T01:00:29Z

Codecov Report

Merging #162 into dev will increase coverage by 1.82%.
The diff coverage is 85.71%.

@@             Coverage Diff             @@
##               dev     #162      +/-   ##
===========================================
+ Coverage     29.7%   31.52%   +1.82%     
- Complexity    1248     1361     +113     
===========================================
  Files           97       97              
  Lines        23305    23312       +7     
  Branches      3871     3872       +1     
===========================================
+ Hits          6923     7350     +427     
+ Misses       15028    14530     -498     
- Partials      1354     1432      +78

Flag	Coverage Δ	Complexity Δ
#JDBC41	`31.31% <85.71%> (+1.71%)`	`1346 <0> (+101)`	✅
#JDBC42	`31.49% <85.71%> (+1.9%)`	`1361 <0> (+117)`	✅

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`35.2% <85.71%> (+3.09%)`	`0 <0> (ø)`	❌
src/main/java/microsoft/sql/DateTimeOffset.java	`37.14% <0%> (-2.86%)`	`8% <0%> (-2%)`
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`44.87% <0%> (-0.95%)`	`183% <0%> (+1%)`
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java	`43.32% <0%> (+0.59%)`	`54% <0%> (+1%)`	✅
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	`23.66% <0%> (+0.66%)`	`36% <0%> (ø)`	❌
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java	`68.68% <0%> (+0.67%)`	`1% <0%> (+1%)`	✅
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	`38.95% <0%> (+0.71%)`	`51% <0%> (+1%)`	✅
...oft/sqlserver/jdbc/SQLServerCallableStatement.java	`13.42% <0%> (+0.83%)`	`39% <0%> (+4%)`	✅
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	`31.79% <0%> (+1.08%)`	`0% <0%> (ø)`	❌
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	`39.92% <0%> (+1.13%)`	`201% <0%> (+11%)`	✅
... and 12 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 369e5e0...a9d41cc. Read the comment docs.

fwi · 2017-02-28T11:47:58Z

I'm sorry but this was not the point of my request. A shared SCHEDULED executor service creates tasks that might be executed in the future instead of starting threads immediately (i.e. threads and tasks are separated, this pull request does not change that). That has the potential to make a real difference in performance. To be really effective, the shared scheduled executor service should be created when the first connection is created and shutdown when the last connection is closed, and is shared/re-used among all connections that need to schedule tasks.

pierresouchay · 2017-02-28T19:25:09Z

src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java

-        timerThread.start();
+        //if ExecutorService is shutdown already, reset it
+        if(executor.isShutdown()){
+            executor = Executors.newCachedThreadPool();


The executor does not create daemon Threads by default, thus the result may be a little different.

I would probably use a thread factory as a parameter that use daemon threads.

xiangyushawn · 2017-02-28T19:56:31Z

@pierresouchay Thank you very much! Thank you for pointing this out. I don't need to worry about shutting down executor manually if I set them as daemon threads.
@fwi thank you for your feedback. You are right, my previous implementation did not improve much. Could you please take a look now and see if that's what you would agree? now, the executor is static final, and re-uses threads for new tasks.

fwi · 2017-02-28T21:46:30Z

Yes, this will improve the situation: threads will get re-used which takes away the relative expensive creating and starting of threads each time. I also had a re-think about my comment on shutdown of the executor service. I think using daemon-threads is sufficient: it will not prevent an application from shutting down and in the case of war-files / redeployable web applications the general advice is to always put the JDBC-drivers in the container (the "common library directory") and not package the JDBC drivers with the war-files - if that advice is followed there will be no memory leak problem either.
Thanks for the improvement!

xiangyushawn · 2017-02-28T21:58:35Z

@fwi Thank you for getting back to us with your thoughts. Again, we really appreciate your suggestions on this improvement. Right now, we are running more tests for this implementation, and will merge the PR after those tests are done. Thank you.

pierresouchay · 2017-02-28T23:47:03Z

src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java

+    private static final ExecutorService executor = Executors.newCachedThreadPool(new ThreadFactory() {
+        @Override
+        public Thread newThread(Runnable r) {
+            Thread t = Executors.defaultThreadFactory().newThread(r);


Another way would be to use a dedicated ThreadGroup (for instance with name SQLServerDriver) and to create threads regarding this behaviour.

For instance:

private static final ExecutorService executor = Executors.newCachedThreadPool(new ThreadFactory() { final ThreadGroup tg = new ThreadGroup("SQLServerDriver"); final AtomicInteger counter = new AtomicInteger(0); @Override public Thread newThread(Runnable r) { Thread t = new Thread(tg, "sqlserverdriver-"+counter.incrementAndGet() t.setDaemon(true); return t; } }

Having a dedicated ThreadGroup and well named threads would ease debug in complex software, such as application servers when there are many threads and would help identifying the ones created by the driver.

(While ThreadGroup may be overkill, at least having a good name for threads help a lot)

Good idea. I will talk to the team and see what name we can give to the thread group. Thank you.

fwi · 2017-03-01T00:47:20Z

src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java

    }

    final void stop() {
+        task.cancel(false);


This must be cancel(true) like in the original implementation (timerThread.interrupt was called). Else new threads will be started. E.g. if 10 queries are fired with a time-out within one second (which is the sleep-interval in the run-method), 10 threads will be created.

Thank you, good catch. I agree. Going to change to true now.

v-nisidh · 2017-03-03T19:49:05Z

@v-xiangs : Can you add Connection Pool test cases on this PR?

fwi · 2017-03-04T12:43:02Z

src/test/java/com/microsoft/sqlserver/jdbc/connection/PoolingTest.java

+            con = ds.getConnection();
+            con.isValid(5);
+
+            pst = con.prepareStatement("SELECT SUSER_SNAME()");


For testing timeout: pst.setQueryTimeout(5);

hello @fwi , could you please inform me what the benefits are by testing setQueryTimeout() instead of isValid()? from my understanding, both create a new thread for the timeout timer

Currently "isValid" ends up using the timeout timer, but this is an implementation choice (other JDBC drivers do it differently for speed and this JDBC driver might one day too) and might change in the future. Also "isValid" is already called by the connection pool before returing a connection from the pool (see for example "validationTimeout" in HikariCP).

Besides that, the original issue was about "queries with a time-out", so why not test that explicitly and make sure to use a query with a time-out?

that makes sense, I will change it.

fwi · 2017-03-04T12:44:11Z

src/test/java/com/microsoft/sqlserver/jdbc/connection/PoolingTest.java

+        config.setJdbcUrl(connectionString);
+        HikariDataSource ds = new HikariDataSource(config);
+
+        connect(ds);


Testing performance with 'trivial queries', so add a loop:

try { int ttStartCount = countTimeoutThreads(); for (int i = 0; i < 100; i++) { connect(ds); } int ttEndCount = countTimeoutThreads(); if (ttEndCount - ttStartCount > 2) { throw new AssertionError("Too many timeout threads started."); } } finally { // always close the datasource ds.close(); }

@fwi : Thanks for suggestions. We are trying out Java MicroBenchmark Harness (JMH) test cases for performance testing. We are figuring out how & when we can open performance test cases. At the same time we want our Travis-CI & Appveyour builds should not be long running.

Bad choice of words on my part: this is actually a functional test. Remember when we changed a little bit of code to prevent creating lots of threads when many queries are executed rapidly? How do you know there is not another mistake like that still in the code? This test simply measures the amount of threads before and after executing many queries rapidly. Measuring is knowing.

On a side note: the datasource backed by the pool implementation does need to be closed when the test is finished. If not, connections remain open and the pool implementation itself starts threads as well that are shutdown when the datasource is closed.

Final note: verify ttEndCount is at least 1 to ensure the test is working (if ttEndCount is zero, the test did not do its intended job).

instead of doing if (ttEndCount - ttStartCount > 2) { throw exception}, I guess only verify ttEndCount is at least 1 is sufficient. In the for loop, if we increase the termination (e.g. to 100000), it may create more than 2 timeout threads.

fwi · 2017-03-04T12:45:01Z

src/test/java/com/microsoft/sqlserver/jdbc/connection/PoolingTest.java

+
+        connect(ds);
+    }
+


Example for thread-counts (untested):

public static int countTimeoutThreads() { ThreadInfo[] tinfos = ManagementFactory.getThreadMXBean().getThreadInfo(ManagementFactory.getThreadMXBean().getAllThreadIds(), 0); // IOBuffer.TimeoutTimer.threadGroupName should be publicly accessible ... final String toname = "mssql-jdbc-TimeoutTimer"; int count = 0; for (ThreadInfo ti : tinfos) { if (ti.getThreadName().startsWith(toname) && !ti.getThreadState().equals(java.lang.Thread.State.TERMINATED)) { count++; } } return count; }

xiangyushawn added 2 commits February 27, 2017 13:59

replace Thread with ExecutorService

d5f10d5

if ExecutorService is shutdown already, reset it

72d596b

fwi mentioned this pull request Feb 28, 2017

TimeoutTimer starts a new Thread for all queries with a query-timeout #150

Closed

pierresouchay reviewed Feb 28, 2017

View reviewed changes

use futures and set threads as Daemon

2979464

pierresouchay reviewed Feb 28, 2017

View reviewed changes

fwi reviewed Mar 1, 2017

View reviewed changes

xiangyushawn added 2 commits February 28, 2017 17:19

thread can be interrupted

585c0b5

group TimeoutTimer threads

4e46838

added Connection Pool test cases

ff2d811

fwi reviewed Mar 4, 2017

View reviewed changes

close datasource, test setQueryTimeout(), count number of threads

ba54360

v-nisidh added this to the 6.1.5 milestone Mar 9, 2017

xiangyushawn added 2 commits March 9, 2017 16:46

add java docs for tests

1780179

add class level java doc

a9d41cc

v-nisidh merged commit e4095e2 into microsoft:dev Mar 10, 2017

xiangyushawn deleted the ExecutorService-instead-of-Thread branch March 10, 2017 01:26

shayaantx mentioned this pull request Jul 21, 2018

TimeoutTimer thread pool max #755

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Executor service instead of thread #162

Executor service instead of thread #162

xiangyushawn commented Feb 28, 2017

codecov-io commented Feb 28, 2017 •

edited

Loading

fwi commented Feb 28, 2017

pierresouchay Feb 28, 2017

xiangyushawn commented Feb 28, 2017

fwi commented Feb 28, 2017

xiangyushawn commented Feb 28, 2017 •

edited

Loading

pierresouchay Feb 28, 2017

xiangyushawn Mar 1, 2017

fwi Mar 1, 2017

xiangyushawn Mar 1, 2017

v-nisidh commented Mar 3, 2017

fwi Mar 4, 2017

xiangyushawn Mar 6, 2017

fwi Mar 7, 2017

xiangyushawn Mar 7, 2017

fwi Mar 4, 2017

v-nisidh Mar 6, 2017

fwi Mar 7, 2017

fwi Mar 7, 2017

fwi Mar 7, 2017

xiangyushawn Mar 7, 2017 •

edited

Loading

fwi Mar 4, 2017

Executor service instead of thread #162

Executor service instead of thread #162

Conversation

xiangyushawn commented Feb 28, 2017

codecov-io commented Feb 28, 2017 • edited Loading

Codecov Report

fwi commented Feb 28, 2017

Choose a reason for hiding this comment

xiangyushawn commented Feb 28, 2017

fwi commented Feb 28, 2017

xiangyushawn commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-nisidh commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangyushawn Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 28, 2017 •

edited

Loading

xiangyushawn commented Feb 28, 2017 •

edited

Loading

xiangyushawn Mar 7, 2017 •

edited

Loading