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

fix: ResultSet#close() should not throw exceptions from session creation #487

Merged
merged 2 commits into from Oct 1, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 1, 2020

If a client application requested a session from the session pool, and that request required a new BatchCreateSessions request that would fail, then the error from the BatchCreateSessions RPC would be propagated to both the ReadContext#executeQuery(..) method as well as the ResultSet#close() method. The latter should not happen, as the close method should silently ignore any errors from the session creation.

olavloite added 2 commits Oct 1, 2020
If a client application requested a session from the session pool, and that request
required a new BatchCreateSessions request that would fail, then the error from the
BatchCreateSessions RPC would be propagated to both the ReadContext#executeQuery(..)
method as well as the ResultSet#close() method. The latter should not happen, as the
close method should silently ignore any errors from the session creation.
@olavloite olavloite requested review from thiagotnunes and skuruppu Oct 1, 2020
@olavloite olavloite requested review from as code owners Oct 1, 2020
@google-cla google-cla bot added the cla: yes label Oct 1, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 1, 2020

Codecov Report

Merging #487 into master will increase coverage by 0.00%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #487   +/-   ##
=========================================
  Coverage     82.20%   82.21%           
- Complexity     2467     2472    +5     
=========================================
  Files           138      138           
  Lines         13631    13643   +12     
  Branches       1312     1314    +2     
=========================================
+ Hits          11206    11217   +11     
- Misses         1896     1898    +2     
+ Partials        529      528    -1     
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/cloud/spanner/SessionPool.java 86.10% <60.00%> (-0.17%) 109.00 <0.00> (ø)
.../com/google/cloud/spanner/ForwardingResultSet.java 100.00% <100.00%> (ø) 7.00 <1.00> (ø)
...a/com/google/cloud/spanner/AsyncResultSetImpl.java 90.66% <0.00%> (-0.78%) 31.00% <0.00%> (ø%)
...cloud/spanner/connection/SingleUseTransaction.java 75.00% <0.00%> (+1.00%) 35.00% <0.00%> (+1.00%)
.../google/cloud/spanner/SpannerExceptionFactory.java 74.35% <0.00%> (+2.56%) 34.00% <0.00%> (+1.00%)

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 81b07e3...9e2df84. Read the comment docs.

@product-auto-label product-auto-label bot added the api: spanner label Oct 1, 2020
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

LGTM

@thiagotnunes thiagotnunes merged commit 60fb986 into master Oct 1, 2020
18 of 19 checks passed
@thiagotnunes thiagotnunes deleted the do-not-propagate-exceptions-in-close branch Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants