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.

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

Process all responses before proceed and fix retry logic for metadata caching #436

Conversation

xiangyushawn
Copy link
Contributor

@xiangyushawn xiangyushawn commented Aug 9, 2017

fix 2 issues in the driver related to metadata caching.

The issues are found by replacing throw with raiserror in the test, in order to run the test against SQL Server 2008

Repo code for first issue:
try_issue.txt

Another issue happens when using batching. The data is inserted/updated more times than expected.

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #436 into dev will increase coverage by 0.06%.
The diff coverage is 89.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #436      +/-   ##
============================================
+ Coverage     46.25%   46.32%   +0.06%     
+ Complexity     2200     2197       -3     
============================================
  Files           108      108              
  Lines         25210    25215       +5     
  Branches       4164     4166       +2     
============================================
+ Hits          11662    11680      +18     
+ Misses        11519    11507      -12     
+ Partials       2029     2028       -1
Flag Coverage Δ Complexity Δ
#JDBC41 46.09% <84.21%> (+0.04%) 2183 <0> (-7) ⬇️
#JDBC42 46.16% <84.21%> (+0.03%) 2193 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 54.06% <100%> (+0.07%) 0 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.65% <85.71%> (+0.28%) 130 <0> (ø) ⬇️
...va/com/microsoft/sqlserver/jdbc/StreamColumns.java 87.83% <0%> (-1.36%) 21% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 44.94% <0%> (-1.13%) 16% <0%> (-1%)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 59.17% <0%> (-0.65%) 88% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.48% <0%> (-0.07%) 238% <0%> (-2%)
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 51% <0%> (+0.08%) 162% <0%> (+1%) ⬆️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 45.83% <0%> (+0.11%) 274% <0%> (+1%) ⬆️
... and 4 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 d4cc533...2c41c21. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #436 into dev will decrease coverage by 0.04%.
The diff coverage is 62.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #436      +/-   ##
============================================
- Coverage     46.25%   46.21%   -0.05%     
- Complexity     2200     2201       +1     
============================================
  Files           108      108              
  Lines         25210    25235      +25     
  Branches       4164     4176      +12     
============================================
  Hits          11662    11662              
- Misses        11519    11555      +36     
+ Partials       2029     2018      -11
Flag Coverage Δ Complexity Δ
#JDBC41 46.03% <58.33%> (-0.02%) 2190 <2> (ø)
#JDBC42 46.08% <58.33%> (-0.06%) 2195 <2> (+2)
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.45% <20%> (-0.47%) 157 <0> (-4)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.39% <73.33%> (+0.02%) 135 <2> (+5) ⬆️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.44% <75%> (-0.54%) 0 <0> (ø)
...java/com/microsoft/sqlserver/jdbc/StringUtils.java 25% <0%> (-12.5%) 3% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 44.94% <0%> (-1.13%) 16% <0%> (-1%)
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 62.44% <0%> (-1.08%) 63% <0%> (+1%)
...va/com/microsoft/sqlserver/jdbc/StreamColInfo.java 78.26% <0%> (-0.91%) 6% <0%> (ø)
...va/com/microsoft/sqlserver/jdbc/StreamTabName.java 85% <0%> (-0.72%) 7% <0%> (ø)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 59.17% <0%> (-0.65%) 88% <0%> (-1%)
... and 19 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 d4cc533...53be4ab. Read the comment docs.

@xiangyushawn
Copy link
Contributor Author

Hello @TobiasSQL , we know you are very busy, but if you could have a few minutes to take a look at this PR, it would be very helpful to us. Thank you 😃

@xiangyushawn xiangyushawn changed the title Process all responses before proceed Process all responses before proceed and fix retry logic for metadata caching Aug 10, 2017
@AfsanehR-zz AfsanehR-zz self-assigned this Aug 22, 2017
@AfsanehR-zz AfsanehR-zz added this to the 6.3.2 milestone Aug 28, 2017
@cheenamalhotra cheenamalhotra modified the milestones: 6.3.2, 6.3.3 Aug 30, 2017
return true;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Method hasNextPacket() content can be compressed to:
return (null!=currentPacket.next);

}
else {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Method endOfCurrentPacket() content can be compressed to:
return (payload == currentPacket.payloadLength);

@xiangyushawn
Copy link
Contributor Author

xiangyushawn commented Sep 13, 2017

Things to verify for this fix:

  • Make sure we don't lose error messages
  • Only retry if the call was "exec sp_execute 14;" and not "INSERT t VALUES(1); exec sp_execute 14;"
  • Manually get the handle and explicitly kill the handle for a specific error type.

@xiangyushawn
Copy link
Contributor Author

close it due to new PR #543

@xiangyushawn xiangyushawn deleted the process-all-responses-before-proceed branch November 17, 2017 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants