Skip to content

Re-introduce Retry logic for prepared statement caching #618

Merged
cheenamalhotra merged 13 commits intomicrosoft:devfrom
AfsanehR-zz:retryLogic
Feb 15, 2018
Merged

Re-introduce Retry logic for prepared statement caching #618
cheenamalhotra merged 13 commits intomicrosoft:devfrom
AfsanehR-zz:retryLogic

Conversation

@AfsanehR-zz
Copy link
Copy Markdown
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
Copy Markdown

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
}
catch (SQLException e) {
if (retryBasedOnFailedReuseOfCachedHandle(e, attempt))
if (retryBasedOnFailedReuseOfCachedHandle(e, attempt) && connection.isStatementPoolingEnabled())
Copy link
Copy Markdown
Contributor

@TobiasSQL TobiasSQL Feb 1, 2018

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
Copy Markdown
Contributor Author

@AfsanehR-zz AfsanehR-zz Feb 1, 2018

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
@@ -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
Copy Markdown
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
Copy Markdown
Contributor Author

@AfsanehR-zz AfsanehR-zz Feb 1, 2018

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
Copy Markdown
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

@ulvii ulvii Awaiting requested review from ulvii

1 more reviewer

@TobiasSQL TobiasSQL TobiasSQL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@AfsanehR-zz @codecov-io @TobiasSQL @lilgreenbird @cheenamalhotra