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.

Sign up for GitHub

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

Jump to bottom

Re-introduce Retry logic for prepared statement caching #618

Merged
merged 13 commits into from
Feb 15, 2018
Merged

Re-introduce Retry logic for prepared statement caching #618

merged 13 commits into from
Feb 15, 2018

Conversation

AfsanehR-zz
Copy link
Contributor

@AfsanehR-zz AfsanehR-zz commented Jan 31, 2018 •
edited
Loading

  • Added APIs for disableStatementPooling
  • Set the disableStatementPooling to true by default
  • Disable prepared statement caching by default
  • Introduce retry logic for missing handle exception.

Fixes issue #610

@codecov-io
Copy link

codecov-io commented Jan 31, 2018 •
edited
Loading

Codecov Report

Merging #618 into dev will decrease coverage by 0.13%.
The diff coverage is 68.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #618      +/-   ##
============================================
- Coverage     46.58%   46.44%   -0.14%     
+ Complexity     2229     2220       -9     
============================================
  Files           109      109              
  Lines         25409    25408       -1     
  Branches       4184     4186       +2     
============================================
- Hits          11836    11801      -35     
- Misses        11539    11569      +30     
- Partials       2034     2038       +4
Flag Coverage Δ Complexity Δ
#JDBC42 46.34% <68.51%> (-0.16%) 2215 <15> (-14)
#JDBC43 46.33% <68.51%> (-0.11%) 2217 <15> (-1)
Impacted Files Coverage Δ Complexity Δ
...t/sqlserver/jdbc/SQLServerConnectionPoolProxy.java 14.65% <ø> (-0.45%) 14 <0> (ø)
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 45.66% <0%> (-0.49%) 66 <0> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.34% <100%> (-0.29%) 130 <0> (-3)
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 76.85% <100%> (ø) 25 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.49% <65.38%> (-0.92%) 155 <2> (-5)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 45.82% <87.5%> (+0.15%) 287 <13> (+9) ⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 42.69% <0%> (-4.5%) 14% <0%> (-3%)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 38.54% <0%> (-1.08%) 42% <0%> (-3%)
...ooglecode/concurrentlinkedhashmap/LinkedDeque.java 27.69% <0%> (-0.77%) 19% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 44.46% <0%> (-0.46%) 104% <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 6c79be8...9ae0843. Read the comment docs.

@AfsanehR-zz AfsanehR-zz requested a review from TobiasSQL January 31, 2018 23:02
@AfsanehR-zz AfsanehR-zz mentioned this pull request Jan 31, 2018
Closed
@AfsanehR-zz AfsanehR-zz requested a review from ulvii February 1, 2018 00:14
TobiasSQL
TobiasSQL suggested changes Feb 1, 2018
View reviewed changes
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java Outdated
@@ -547,7 +547,7 @@ final void doExecutePreparedStatement(PrepStmtExecCmd command) throws SQLServerE
getNextResult();
}
catch (SQLException e) {
if (retryBasedOnFailedReuseOfCachedHandle(e, attempt))
if (retryBasedOnFailedReuseOfCachedHandle(e, attempt) && connection.isStatementPoolingEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest changing retryBasedOnFailedReuseOfCachedHandle to take care of the connection.IsStatementPoolingEnabled() checking, that is part of whether or not to reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

TobiasSQL
TobiasSQL suggested changes Feb 1, 2018
View reviewed changes
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java Outdated
@@ -565,13 +566,18 @@ else if (EXECUTE_UPDATE == executeMethod && null != resultSet) {

/** Should the execution be retried because the re-used cached handle could not be re-used due to server side state changes? */
private boolean retryBasedOnFailedReuseOfCachedHandle(SQLException e,
Copy link
Contributor

@TobiasSQL TobiasSQL Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting here as well as the method changed. Suggest changing retryBasedOnFailedReuseOfCachedHandle to take care of the connection.IsStatementPoolingEnabled() check which is part of whether or not to reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Thanks!

@cheenamalhotra cheenamalhotra added this to the 6.4.0 milestone Feb 1, 2018
TobiasSQL
TobiasSQL reviewed Feb 2, 2018
View reviewed changes
Copy link
Contributor

@TobiasSQL TobiasSQL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any tests of the actual retry logic?

@cheenamalhotra cheenamalhotra merged commit a64b9ef into microsoft:dev Feb 15, 2018
@TobiasSQL TobiasSQL mentioned this pull request Feb 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TobiasSQL TobiasSQL TobiasSQL approved these changes

@ulvii ulvii Awaiting requested review from ulvii

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@AfsanehR-zz @codecov-io @TobiasSQL @cheenamalhotra