Skip to content

fix: wrong use of getRetryDelayInMillis() / 1000 in documentation and retry loops #885

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
Merged
merged 5 commits into from
Feb 19, 2021
Merged

fix: wrong use of getRetryDelayInMillis() / 1000 in documentation and retry loops #885

merged 5 commits into from
Feb 19, 2021

Conversation

olavloite
Copy link
Collaborator

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 a team as a code owner February 17, 2021 08:55
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Feb 17, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 17, 2021
@codecov
Copy link

codecov bot commented Feb 17, 2021 •
edited
Loading

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
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Ick! This is worth a very quick release of 4.0.1

thiagotnunes
thiagotnunes approved these changes Feb 17, 2021
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java Outdated
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

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
Collaborator Author

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 Merge the pull request once unit tests and other checks pass. label Feb 19, 2021
@olavloite
Copy link
Collaborator Author

(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
@gcf-merge-on-green gcf-merge-on-green bot deleted the transaction-manager-documentation branch February 19, 2021 08:44
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 19, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 23, 2021
@release-please
chore(master): release 4.0.1 (#883)
9332efb 
🤖 I have created a release \*beep\* \*boop\* 
---
### [4.0.1](https://www.github.com/googleapis/java-spanner/compare/v4.0.0...v4.0.1) (2021-02-22)


### Bug Fixes

* wrong use of getRetryDelayInMillis() / 1000 in documentation and retry loops ([#885](https://www.github.com/googleapis/java-spanner/issues/885)) ([a55d7ce](https://www.github.com/googleapis/java-spanner/commit/a55d7ce64fff434151c1c3af0796d290e9db7470)), closes [#874](https://www.github.com/googleapis/java-spanner/issues/874)


### Documentation

* Add OpenCensus to OpenTelemetry shim to README ([#879](https://www.github.com/googleapis/java-spanner/issues/879)) ([b58d73d](https://www.github.com/googleapis/java-spanner/commit/b58d73ddb768c0d33d149ed8bc84f5af618514e1))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.19.0 ([#895](https://www.github.com/googleapis/java-spanner/issues/895)) ([e3e2c95](https://www.github.com/googleapis/java-spanner/commit/e3e2c95936f40a7954639a95c84cc9495e318e55))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
@yoshi-automation
chore(all): auto-regenerate .pb.go files (googleapis#885)
2345490 
This is an auto-generated regeneration of the .pb.go files by
cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will
update the corresponding PR to depend on the newer version of go-genproto, and
assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot
will not create any more regeneration PRs. If all regen PRs are closed,
gapicgen will create a new set of regeneration PRs once per night.

If you have been assigned to review this PR, please:

- Ensure that CI is passing. If it's failing, it requires your manual attention.
- Approve and submit this PR if you believe it's ready to ship. That will prompt
genbot to assign reviewers to the google-cloud-go PR.

Corresponding google-cloud-go PR: googleapis/google-cloud-go#6553

Changes:

chore: regenerate API index

  Source-Link: googleapis/googleapis@f3e6b9f

feat(datastream): added support for BigQuery destination and PostgreSQL source types
  PiperOrigin-RevId: 469482613
  Source-Link: googleapis/googleapis@441339a

chore: regenerate API index

  Source-Link: googleapis/googleapis@67e514e

feat(aiplatform): add UpsertDatapoints and RemoveDatapoints rpcs to IndexService in aiplatform v1 index_service.proto
  PiperOrigin-RevId: 469481982
  Source-Link: googleapis/googleapis@e4fe55a

chore: regenerate API index

  Source-Link: googleapis/googleapis@95e44ae

feat(aiplatform): add UpsertDatapoints and RemoveDatapoints rpcs to IndexService in aiplatform v1beta1 index_service.proto
  PiperOrigin-RevId: 469481692
  Source-Link: googleapis/googleapis@624cc45

chore: regenerate API index

  Source-Link: googleapis/googleapis@ebed4ae

feat(vpcaccess): Adds support for configuring scaling settings
  Clients can now specify machine type and min/max instances when creating a connector.

  PiperOrigin-RevId: 469463049
  Source-Link: googleapis/googleapis@17b39f1

chore: regenerate API index

  Source-Link: googleapis/googleapis@fcd2684

feat(retail): release Control and ServingConfig serivces to v2 version feat: release AttributesConfig APIs to v2 version feat: release CompletionConfig APIs to v2 version feat: add local inventories info to the Product resource docs: Improved documentation for Fullfillment and Inventory API in ProductService docs: minor documentation format and typo fixes
  PiperOrigin-RevId: 469399525
  Source-Link: googleapis/googleapis@e9bcb6c
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
@renovate-bot
chore(deps): update dependency com.google.cloud:google-cloud-spanner-…
5708aa2 
…jdbc to v2.7.1 (googleapis#885)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagotnunes thiagotnunes thiagotnunes approved these changes

@elharo elharo elharo approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Thread.sleep is one 1000 times too fast
3 participants
@olavloite @thiagotnunes @elharo