Marked exceptions for create and deletes in DNS as non-idempotent. #883

mderka · 2016-04-08T23:11:45Z

Fixes #808, relates to #816 .

coveralls · 2016-04-08T23:18:16Z

Coverage remained the same at 85.597% when pulling 35d9aee on mderka:sockettimeout into af18b50 on GoogleCloudPlatform:master.

mderka · 2016-04-09T00:55:04Z

@aozarov PTAL

coveralls · 2016-04-09T01:02:01Z

Coverage increased (+0.006%) to 85.603% when pulling ae3e446 on mderka:sockettimeout into af18b50 on GoogleCloudPlatform:master.

coveralls · 2016-04-09T01:10:02Z

Coverage decreased (-0.004%) to 85.593% when pulling ae3e446 on mderka:sockettimeout into af18b50 on GoogleCloudPlatform:master.

gcloud-java-core/src/main/java/com/google/gcloud/BaseServiceException.java

-            && (retryableError.reason() == null || retryableError.reason().equals(this.reason()))) {
+        boolean match = (retryableError.code() == null || retryableError.code().equals(this.code()))
+            && (retryableError.reason() == null || retryableError.reason().equals(this.reason()));
+        if (match && idempotent || match && retryableError.rejected()) {


aozarov · 2016-04-09T17:41:00Z

Looks good! Few comments/suggestions for further cleanup.

coveralls · 2016-04-11T16:39:46Z

Coverage decreased (-0.03%) to 85.563% when pulling 2d198d0 on mderka:sockettimeout into af18b50 on GoogleCloudPlatform:master.

mderka · 2016-04-11T16:49:29Z

All done. PTAL

gcloud-java-core/src/main/java/com/google/gcloud/BaseServiceException.java

+      this(code, reason, false);
+    }
+
+    public Error(Integer code, String reason, boolean rejected) {


garrettjonesgoogle · 2016-04-12T16:34:09Z

Hi,

I wanted to remark that create and delete are actually idempotent, and changing them to non-idempotent miscategorizes them. Create is idempotent because after the first or subsequent calls, the result is already "X exists"; Delete is idempotent because after the first or subsequent calls, the result is "X does not exist". The success or error returned to the client is a separate issue from the idempotency of the operation on the data.

Here is an alternative that is slightly more complex, but would probably do what the client expects:

On the first call, if the result is ALREADY_EXISTS, then fail with that.
On the first call, if the result is a different but retryable error code, then do a retry.
On the retry, if the result is ALREADY_EXISTS, then return success. (The assumption is that the first failure happened after successful creation on the server)

The only case that might be odd is if there is a failure to contact the service, but the entity was already created before, then the result will be a success even though per service semantics it should return ALREADY_EXISTS. I would imagine this should be pretty uncommon though, and should probably only happen when the entity was only very recently created.

gcloud-java-core/src/main/java/com/google/gcloud/BaseServiceException.java

@@ -138,7 +146,7 @@ public BaseServiceException(int code, String message, String reason, boolean ide
    this.code = code;
    this.reason = reason;
    this.idempotent = idempotent;
-    this.retryable = idempotent && new Error(code, reason).isRetryable(retryableErrors());
+    this.retryable = new Error(code, reason).isRetryable(idempotent, retryableErrors());


aozarov · 2016-04-14T03:21:50Z

@garrettjonesgoogle I agree conceptually with what you are suggesting (see my other comment). However for create I am not convinced that it is OK to hide from the user the fact that his "Create" was actually successful (it was not there before) or that it was already created (maybe with different values then provided).

I don't think that "The assumption is that the first failure happened after successful creation on the server" is always correct (it could have been created by a parallel call).

garrettjonesgoogle · 2016-04-14T15:39:49Z

@aozarov Good point about the entity possibly being created with different values than the current request. One option there is when the situation is ambiguous, then do a get call to the server to get the existing entity, and determine if the values are the same, and report back appropriately. It's definitely higher overhead and more complex (you will have made a minimum of 3 RPC calls - 1 failed create, one create resulting in already_exists, and 1 get call), but it would be more correct.

mderka · 2016-04-14T18:53:44Z

I created issue #924 to continue with the discussion.

I removed the conflicts and incorporated the comments. Ready to proceed.

coveralls · 2016-04-14T19:00:49Z

Coverage decreased (-0.004%) to 85.566% when pulling f81a293 on mderka:sockettimeout into 13ab36f on GoogleCloudPlatform:master.

coveralls · 2016-04-14T21:42:48Z

Coverage decreased (-0.02%) to 85.554% when pulling 2a11dbf on mderka:sockettimeout into 13ab36f on GoogleCloudPlatform:master.

* feat: Enable REST transport for most of Java and Go clients PiperOrigin-RevId: 456641589 Source-Link: googleapis/googleapis@8a251f5 Source-Link: https://github.com/googleapis/googleapis-gen/commit/4ca52a529cf01308d9714950edffbea3560cfbdb Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNGNhNTJhNTI5Y2YwMTMwOGQ5NzE0OTUwZWRmZmJlYTM1NjBjZmJkYiJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * deps: adding gax-httpjson * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Tomo Suzuki <[email protected]>

…883) * chore(deps): update dependency com.google.cloud:libraries-bom to v26 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Source-Link: googleapis/synthtool@7336562 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:d4b80feffe1579818cdc39466152e9de95789a193408506cd4a1ffbe8804dc00

🤖 I have created a release *beep* *boop* --- ## [3.3.2](googleapis/java-monitoring@v3.3.1...v3.3.2) (2022-07-13) ### Bug Fixes * enable longpaths support for windows test ([#1485](https://github.com/googleapis/java-monitoring/issues/1485)) ([#883](googleapis/java-monitoring#883)) ([0aa8d72](googleapis/java-monitoring@0aa8d72)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…1.1 (#883) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.1.0` -> `26.1.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/compatibility-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/confidence-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-language).

🤖 I have created a release *beep* *boop* --- ## [3.0.7](googleapis/java-recaptchaenterprise@v3.0.6...v3.0.7) (2022-09-08) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.2 ([#882](googleapis/java-recaptchaenterprise#882)) ([26e9931](googleapis/java-recaptchaenterprise@26e9931)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

…1.4 (#883) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.1.3` -> `26.1.4` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.4/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.4/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.4/compatibility-slim/26.1.3)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.4/confidence-slim/26.1.3)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-scheduler).

…onfig to v1.5.3 (#883) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:google-cloud-shared-config](https://togithub.com/googleapis/java-shared-config) | `1.5.2` -> `1.5.3` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-config/1.5.3/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-config/1.5.3/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-config/1.5.3/compatibility-slim/1.5.2)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-config/1.5.3/confidence-slim/1.5.2)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/java-shared-config</summary> ### [`v1.5.3`](https://togithub.com/googleapis/java-shared-config/blob/HEAD/CHANGELOG.md#153-httpsgithubcomgoogleapisjava-shared-configcomparev152v153-2022-07-27) [Compare Source](https://togithub.com/googleapis/java-shared-config/compare/v1.5.2...v1.5.3) ##### Dependencies - update dependency org.junit.vintage:junit-vintage-engine to v5.9.0 ([#497](https://togithub.com/googleapis/java-shared-config/issues/497)) ([1fc6ab4](https://togithub.com/googleapis/java-shared-config/commit/1fc6ab445624cd4f9c8b161d109f346a9e5ed09a)) - Update doclet to latest version 1.6.0 ([38fb7c3](https://togithub.com/googleapis/java-shared-config/commit/38fb7c3957fb6c9b2da10f9e463cc93a8b80a3a4)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).

chore: remove check for flatten plugin We have had the check for the flatten-maven-plugin in each Cloud Java client repository. However, the behavior of the plugin has been stable and its not each repository's responsibility to assert the plugin's behavior. A new check is going to be added at the googleapis/java-shared-config repository to assert the plugin's behavior when we upgrade its version. Source-Link: https://togithub.com/googleapis/synthtool/commit/9266ddc3b17fc15f34d2fb88ce8c5f1a4bfe64b0 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:ae72564aa9c368b9ccd96f4af21f87889fd83b9e60635b80844deb5a2ccd08aa

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 8, 2016

mderka added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 9, 2016

mderka force-pushed the sockettimeout branch from 427bbbf to ae3e446 Compare April 9, 2016 00:54

mderka removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 9, 2016

aozarov reviewed Apr 9, 2016
View reviewed changes

aozarov mentioned this pull request Apr 9, 2016

RetryHelper can be configured to not retry on socket timeouts. #882

Closed

aozarov added api: core api: dns Issues related to the Cloud DNS API. labels Apr 9, 2016

aozarov mentioned this pull request Apr 9, 2016

Update pubsub client with latest surface changes. #885

Merged

mziccard reviewed Apr 12, 2016
View reviewed changes

aozarov reviewed Apr 14, 2016
View reviewed changes

mderka added 4 commits April 14, 2016 11:38

Marked exceptions for create and deletes as non-idempotent.

7a20c01

Added error flag for indicating if the request was rejected.

81362af

Minor cleanup refactoring.

3b8631f

Minor code cleanup.

c3647d0

mderka mentioned this pull request Apr 14, 2016

Idempotent methods: hide exceptions on retries if there is evidence the original operation succeeded #924

Closed

mderka force-pushed the sockettimeout branch from 2d198d0 to f81a293 Compare April 14, 2016 18:52

Switched create back to idempotent, comment adjustments

2a11dbf

mderka force-pushed the sockettimeout branch from f81a293 to 2a11dbf Compare April 14, 2016 21:34

aozarov merged commit fd740ea into googleapis:master Apr 14, 2016

aozarov mentioned this pull request Apr 14, 2016

Create a retry helper that doesn't retry on socket timeouts #816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marked exceptions for create and deletes in DNS as non-idempotent. #883

Marked exceptions for create and deletes in DNS as non-idempotent. #883

mderka commented Apr 8, 2016

coveralls commented Apr 8, 2016

mderka commented Apr 9, 2016

coveralls commented Apr 9, 2016

coveralls commented Apr 9, 2016

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Apr 9, 2016

coveralls commented Apr 11, 2016

mderka commented Apr 11, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Apr 12, 2016

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Apr 14, 2016

garrettjonesgoogle commented Apr 14, 2016

mderka commented Apr 14, 2016

coveralls commented Apr 14, 2016

coveralls commented Apr 14, 2016

Marked exceptions for create and deletes in DNS as non-idempotent. #883

Marked exceptions for create and deletes in DNS as non-idempotent. #883

Conversation

mderka commented Apr 8, 2016

coveralls commented Apr 8, 2016

mderka commented Apr 9, 2016

coveralls commented Apr 9, 2016

coveralls commented Apr 9, 2016

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Apr 9, 2016

coveralls commented Apr 11, 2016

mderka commented Apr 11, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Apr 12, 2016

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Apr 14, 2016

garrettjonesgoogle commented Apr 14, 2016

mderka commented Apr 14, 2016

coveralls commented Apr 14, 2016

coveralls commented Apr 14, 2016