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: always delete all backups from an owned test instance #557

Merged
merged 2 commits into from Oct 27, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 27, 2020

An instance can only be deleted once all backups on the instance have been deleted. The integration test environment should therefore first delete all backups on an owned instance before trying to delete the instance itself.

Individual test cases are responsible for deleting any backups that are created during the test, but sometimes backup creation operations do not finish within reasonable time. The deletion of these backups by the individual test cases sometimes seem to fail. This change is an additional fallback for making sure that all backups that were created during the tests are removed, and that the test instance can be removed.

Fixes #542

An instance can only be deleted once all backups on the instance have been
deleted. The integration test environment should therefore first delete all
backups on an owned instance before trying to delete the instance itself.

Fixes #542
@olavloite olavloite requested a review from as a code owner Oct 27, 2020
@google-cla google-cla bot added the cla: yes label Oct 27, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 27, 2020

Codecov Report

Merging #557 into master will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #557      +/-   ##
============================================
+ Coverage     83.93%   84.03%   +0.10%     
- Complexity     2501     2503       +2     
============================================
  Files           141      141              
  Lines         13766    13806      +40     
  Branches       1315     1317       +2     
============================================
+ Hits          11554    11602      +48     
+ Misses         1667     1657      -10     
- Partials        545      547       +2     
Impacted Files Coverage Δ Complexity Δ
...gle/cloud/spanner/AsyncTransactionManagerImpl.java 70.42% <0.00%> (-1.01%) 12.00% <0.00%> (ø%)
...ain/java/com/google/cloud/spanner/SessionPool.java 88.13% <0.00%> (+1.23%) 71.00% <0.00%> (ø%)
...ud/spanner/SessionPoolAsyncTransactionManager.java 86.55% <0.00%> (+1.64%) 11.00% <0.00%> (+2.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 d92b7c2...5cf721e. Read the comment docs.

@olavloite olavloite requested review from hengfengli and lesv Oct 27, 2020
Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM. Thanks for fixing this issue.

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Thanks for the fix!

@thiagotnunes thiagotnunes merged commit ff571b0 into master Oct 27, 2020
21 checks passed
@thiagotnunes thiagotnunes deleted the delete-backups-from-test-instance branch Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants