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: wrong use of getRetryDelayInMillis() / 1000 in documentation and retry loops #885

Merged
merged 5 commits into from Feb 19, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Feb 17, 2021

Both the documentation for TransactionManager as well as some internal retry loops wrongly used SpannerException#getRetryDelayInMillis() / 1000 as input for Thread.sleep(long). The retry delay is already in milliseconds and should not be modified.

Fixes #874

@olavloite olavloite requested a review from thiagotnunes Feb 17, 2021
@olavloite olavloite requested a review from as a code owner Feb 17, 2021
@product-auto-label product-auto-label bot added the api: spanner label Feb 17, 2021
@google-cla google-cla bot added the cla: yes label Feb 17, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 17, 2021

Codecov Report

Merging #885 (64c9c1f) into master (1f9f76a) will decrease coverage by 0.00%.
The diff coverage is 92.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #885      +/-   ##
============================================
- Coverage     85.20%   85.19%   -0.01%     
- Complexity     2620     2623       +3     
============================================
  Files           145      145              
  Lines         14262    14287      +25     
  Branches       1378     1379       +1     
============================================
+ Hits          12152    12172      +20     
- Misses         1538     1539       +1     
- Partials        572      576       +4     
Impacted Files Coverage Δ Complexity Δ
...cloud/spanner/connection/ReadWriteTransaction.java 81.61% <33.33%> (-0.41%) 55.00 <0.00> (ø)
...om/google/cloud/spanner/TransactionRunnerImpl.java 85.98% <94.44%> (-0.70%) 10.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java 89.26% <100.00%> (+0.22%) 73.00 <0.00> (+1.00)
.../google/cloud/spanner/SpannerExceptionFactory.java 84.68% <100.00%> (+2.21%) 47.00 <1.00> (+1.00)
...m/google/cloud/spanner/connection/SpannerPool.java 87.36% <0.00%> (-0.53%) 33.00% <0.00%> (ø%)
...a/com/google/cloud/spanner/SessionPoolOptions.java 69.53% <0.00%> (ø) 18.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 1f9f76a...086fdc6. Read the comment docs.

elharo
elharo approved these changes Feb 17, 2021
elharo
elharo approved these changes Feb 17, 2021
Copy link
Contributor

@elharo elharo left a comment

Ick! This is worth a very quick release of 4.0.1

ErrorCode.ABORTED, "Aborted due to failed initial statement", e));
ErrorCode.ABORTED,
"Aborted due to failed initial statement",
SpannerExceptionFactory.createAbortedExceptionWithRetryDelay(null, e, 0, 1)));
Copy link
Contributor

@thiagotnunes thiagotnunes Feb 17, 2021

Choose a reason for hiding this comment

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

Should we add a non null message here?

Copy link
Contributor Author

@olavloite olavloite Feb 19, 2021

Choose a reason for hiding this comment

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

Yes, that's probably better to keep it in line with the other manually constructed Aborted errors.

@olavloite olavloite added the automerge label Feb 19, 2021
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Feb 19, 2021

(Failed samples build is a known flaky test case)

@gcf-merge-on-green gcf-merge-on-green bot merged commit a55d7ce into master Feb 19, 2021
16 of 17 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the transaction-manager-documentation branch Feb 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Feb 19, 2021
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.

3 participants