Conversation

olavloite

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

@olavloiteolavloite requested a review from a team as a code owner February 17, 2021 08:55
@product-auto-labelproduct-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Feb 17, 2021
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Feb 17, 2021
@codecov

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 FilesCoverage ΔComplexity Δ
...cloud/spanner/connection/ReadWriteTransaction.java81.61% <33.33%> (-0.41%)55.00 <0.00> (ø)
...om/google/cloud/spanner/TransactionRunnerImpl.java85.98% <94.44%> (-0.70%)10.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java89.26% <100.00%> (+0.22%)73.00 <0.00> (+1.00)
.../google/cloud/spanner/SpannerExceptionFactory.java84.68% <100.00%> (+2.21%)47.00 <1.00> (+1.00)
...m/google/cloud/spanner/connection/SpannerPool.java87.36% <0.00%> (-0.53%)33.00% <0.00%> (ø%)
...a/com/google/cloud/spanner/SessionPoolOptions.java69.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

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.

@olavloiteolavloite added the automergeMerge the pull request once unit tests and other checks pass.label Feb 19, 2021
@olavloite

(Failed samples build is a known flaky test case)

@gcf-merge-on-greengcf-merge-on-green bot merged commit a55d7ce into master Feb 19, 2021
@gcf-merge-on-greengcf-merge-on-green bot deleted the transaction-manager-documentation branch February 19, 2021 08:44
@gcf-merge-on-greengcf-merge-on-green bot removed the automergeMerge 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..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..com/googleapis/java-spanner/issues/885)) ([a55d7ce](https://www..com/googleapis/java-spanner/commit/a55d7ce64fff434151c1c3af0796d290e9db7470)), closes [#874](https://www..com/googleapis/java-spanner/issues/874)


### Documentation

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


### Dependencies

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


This PR was generated with [Release Please](https://.com/googleapis/release-please). See [documentation](https://.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 . Already have an account? Sign in to comment

@thiagotnunes thiagotnunes thiagotnunes approved these changes

@elharo elharo elharo approved these changes

No one assigned
api: spannerIssues related to the googleapis/java-spanner API.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.

Thread.sleep is one 1000 times too fast