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

Fixed issues in BulkCopy exception handling #801

Merged
merged 2 commits into from Sep 24, 2018
Merged

Fixed issues in BulkCopy exception handling #801

merged 2 commits into from Sep 24, 2018

Conversation

ulvii
Copy link
Contributor

@ulvii ulvii commented Sep 15, 2018 •
edited

  1. Removing the catch block, because the driver should not blindly send attention packets. To cancel a query, attention packets should only be sent if the client has already sent a request, which has the last packet with EOM bit (0x01) set in status. If a complete request has not been sent to the server, the driver MUST send the next packet with both ignore bit (0x02) and EOM bit (0x01) set in the status to cancel the request (2.2.1.7 of TDS Technical Documentation). This is already properly handled in IOBuffer.execute(), which is the only method that calls doInsertBulk().
  2. Adding this.tdsMessageType == TDS.PKT_BULK, because prior to TDS.PKT_BULK, the driver has already sent INSERT BULK SQL statement to the server, which in case of an exception needs to be cancelled too.

@ulvii ulvii added this to the 7.1.1 milestone Sep 17, 2018
@cheenamalhotra cheenamalhotra added this to Under Peer Review in MSSQL JDBC Sep 17, 2018
@codecov-io
Copy link

Codecov Report

Merging #801 into dev will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##                dev    #801      +/-   ##
===========================================
+ Coverage     48.11%   48.2%   +0.08%     
+ Complexity     2783    2781       -2     
===========================================
  Files           116     116              
  Lines         27862   27853       -9     
  Branches       4641    4640       -1     
===========================================
+ Hits          13407   13426      +19     
+ Misses        12235   12203      -32     
- Partials       2220    2224       +4
Flag Coverage Δ Complexity Δ
#JDBC42 47.71% <100%> (+0.08%) 2738 <0> (-3) ⬇️
#JDBC43 48% <100%> (-0.03%) 2773 <0> (-2)
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.58% <ø> (-0.08%) 262 <0> (-1)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 54.09% <100%> (+0.62%) 0 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 32.38% <0%> (-0.31%) 253% <0%> (-3%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.25% <0%> (-0.18%) 0% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.61% <0%> (-0.1%) 135% <0%> (-1%)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 39% <0%> (+0.21%) 43% <0%> (ø) ⬇️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 60.9% <0%> (+0.64%) 88% <0%> (+1%) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 45.73% <0%> (+1.96%) 107% <0%> (+1%) ⬆️
... and 1 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 0a1759f...a84e519. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #801 into dev will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #801      +/-   ##
============================================
+ Coverage     48.11%   48.18%   +0.06%     
- Complexity     2783     2785       +2     
============================================
  Files           116      116              
  Lines         27862    27853       -9     
  Branches       4641     4640       -1     
============================================
+ Hits          13407    13422      +15     
+ Misses        12235    12211      -24     
  Partials       2220     2220
Flag Coverage Δ Complexity Δ
#JDBC42 47.71% <100%> (+0.08%) 2745 <0> (+4) ⬆️
#JDBC43 48.06% <100%> (+0.03%) 2775 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.46% <ø> (-0.2%) 263 <0> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.72% <100%> (+0.26%) 0 <0> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 79.28% <0%> (-0.2%) 4% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.49% <0%> (+0.05%) 0% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 46.38% <0%> (+2.62%) 108% <0%> (+2%) ⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 46.15% <0%> (+3.29%) 16% <0%> (+1%) ⬆️

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 0a1759f...a84e519. Read the comment docs.

2 similar comments
@codecov-io
Copy link

Codecov Report

Merging #801 into dev will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #801      +/-   ##
============================================
+ Coverage     48.11%   48.18%   +0.06%     
- Complexity     2783     2785       +2     
============================================
  Files           116      116              
  Lines         27862    27853       -9     
  Branches       4641     4640       -1     
============================================
+ Hits          13407    13422      +15     
+ Misses        12235    12211      -24     
  Partials       2220     2220
Flag Coverage Δ Complexity Δ
#JDBC42 47.71% <100%> (+0.08%) 2745 <0> (+4) ⬆️
#JDBC43 48.06% <100%> (+0.03%) 2775 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.46% <ø> (-0.2%) 263 <0> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.72% <100%> (+0.26%) 0 <0> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 79.28% <0%> (-0.2%) 4% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.49% <0%> (+0.05%) 0% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 46.38% <0%> (+2.62%) 108% <0%> (+2%) ⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 46.15% <0%> (+3.29%) 16% <0%> (+1%) ⬆️

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 0a1759f...a84e519. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #801 into dev will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #801      +/-   ##
============================================
+ Coverage     48.11%   48.18%   +0.06%     
- Complexity     2783     2785       +2     
============================================
  Files           116      116              
  Lines         27862    27853       -9     
  Branches       4641     4640       -1     
============================================
+ Hits          13407    13422      +15     
+ Misses        12235    12211      -24     
  Partials       2220     2220
Flag Coverage Δ Complexity Δ
#JDBC42 47.71% <100%> (+0.08%) 2745 <0> (+4) ⬆️
#JDBC43 48.06% <100%> (+0.03%) 2775 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.46% <ø> (-0.2%) 263 <0> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.72% <100%> (+0.26%) 0 <0> (ø) ⬇️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 79.28% <0%> (-0.2%) 4% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.49% <0%> (+0.05%) 0% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 46.38% <0%> (+2.62%) 108% <0%> (+2%) ⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 46.15% <0%> (+3.29%) 16% <0%> (+1%) ⬆️

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 0a1759f...a84e519. Read the comment docs.

peterbae
peterbae approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@peterbae peterbae left a comment

Choose a reason for hiding this comment

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

I looked pretty vigorously into this to see if the changes break anything, but I couldn't find anything. lgtm

@ulvii ulvii merged commit ca7141f into microsoft:dev Sep 24, 2018
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Sep 24, 2018
@ulvii ulvii deleted the BulkCopyErrorHandling branch September 24, 2018 19:44
@ulvii ulvii mentioned this pull request Oct 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rene-ye rene-ye rene-ye approved these changes

@lilgreenbird lilgreenbird lilgreenbird approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@peterbae peterbae peterbae approved these changes

Assignees
No one assigned
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Milestone
7.1.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ulvii @codecov-io @rene-ye @lilgreenbird @cheenamalhotra @peterbae