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.

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

fix: throw already exists exception when op exists #762

Closed
wants to merge 1 commit into from
Closed

fix: throw already exists exception when op exists #762

wants to merge 1 commit into from

Conversation

thiagotnunes
Copy link
Contributor

All the other libraries and the rest API return / throw an error when the same operation id is used. In java, we used to piggy back on the existing operation instead. Here, we change the behaviour to be consistent with the other implementations.

@thiagotnunes
fix: throw already exists exception when op exists
2509211 
All the other libraries and the rest API return / throw an error when
the same operation id is used. In java, we used to piggy back on the
existing operation instead. Here, we change the behaviour to be
consistent with the other implementations.
@thiagotnunes thiagotnunes requested a review from a team as a code owner December 21, 2020 06:09
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Dec 21, 2020
olavloite
olavloite requested changes Dec 21, 2020
View reviewed changes
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
if (se instanceof AdminRequestsPerMinuteExceededException) {
// Propagate this to trigger a retry.
throw se;
}
if (t instanceof AlreadyExistsException) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that this looks strange, but this is actually an intended feature that has been in the client library for a very long time. The following is what is going on here:

  1. End users can supply an operation ID manually in the public API:

    java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java

    Line 194 in 2509211

    @Nullable String operationId)
  2. The generated code and retry settings make the UpdateDatabaseDdl method retryable:

    java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/DatabaseAdminStubSettings.java

    Line 865 in 2509211

    .setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("retry_policy_0_codes"))
  3. This means that gax will automatically retry the RPC if it fails with for example UNAVAILABLE. If the user specified an operation ID and the UNAVAILABLE error occurred because the response from the server did not reach the client, the operation will already have started on the server and would cause an AlreadyExists error.
  4. The AlreadyExists error from 3 should therefore not be propagated, but used to resume the tracking of the already existing operation.

Changing this would therefore technically be a breaking change as users might be relying on this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification @olavloite. Will discuss it with the team on how to proceed.

@thiagotnunes thiagotnunes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 22, 2020
@thiagotnunes thiagotnunes requested a review from a team as a code owner December 22, 2020 22:58
@thiagotnunes
Copy link
Contributor Author

Closing in favour of #767

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#762)
d576998 
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#5667

Changes:

docs(securitycenter): Update documentation for the Finding resource field "project_display_name" docs: Update documentation for the Mute fields on Findings
  PiperOrigin-RevId: 429148908
  Source-Link: googleapis/googleapis@c93764c

feat(dataproc)!: add support for Virtual Dataproc cluster running on GKE cluster
  Committer: @Padmaar
  PiperOrigin-RevId: 429111624
  Source-Link: googleapis/googleapis@da999a2

chore(appengine): cleanup unused proto imports
  PiperOrigin-RevId: 429104913
  Source-Link: googleapis/googleapis@62d0e05

docs(aiplatform): fix misformatted field description
  PiperOrigin-RevId: 429098186
  Source-Link: googleapis/googleapis@e75c527

fix(spanner/admin/instance)!: annotating some fields as REQUIRED
  These fields were actually always required by the backend, so annotation just documents status quo. I believe this change will not require major version bump for any language.

  PiperOrigin-RevId: 429093810
  Source-Link: googleapis/googleapis@dc04c1c

chore: regenerate API index

  Source-Link: googleapis/googleapis@1bbb5df

feat(dataplex): added client side library for the followings: 1. Content APIs. 2. Create|Update|Delete Metadata APIs (e.g. Entity and/or Partition).
  PiperOrigin-RevId: 429081053
  Source-Link: googleapis/googleapis@7b42fd0

feat(ruby): Update google-cloud-compute-v1 dependency to 1.1.0 or later

  Source-Link: googleapis/googleapis@f4b9e64

chore: Update rules_gapic Bazel rules to 0.12.0
  PiperOrigin-RevId: 429071255
  Source-Link: googleapis/googleapis@17ca037

chore(automl): cleanup unused protos in google/cloud/automl/v1
  PiperOrigin-RevId: 429068183
  Source-Link: googleapis/googleapis@ed90e7c

feat(osconfig/agentendpoint): Add field to PatchConfig message:   - mig_instances_allowed fix: Add NONE Interpreter enum value that should be used over INTERPRETER_UNSPECIFIED in ExecStepConfig message
  PiperOrigin-RevId: 429025668
  Source-Link: googleapis/googleapis@2b26aaa
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
@release-please
chore(main): release 2.6.1-SNAPSHOT (googleapis#762)
4356c88 
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite requested 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. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thiagotnunes @olavloite