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: treat kDeadlineExceeded as permanent error #8525

Merged
merged 6 commits into from
Mar 12, 2022

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Mar 10, 2022

I did not change all of them. I left BigQuery, Bigtable Admin, IAM, and Spanner Admin retry traits as they were. The thinking was that these libraries were more mature. (I changed Logging because it is not GA).

We might want to talk about this...

(Oh and the diff looks weird for GKE Hub / Gaming because I rearranged the order)


This change is Reviewable

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: a548f6b809cf29f8a03bdeb9873bf22fff1602b7

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #8525 (fa5b655) into main (25f6b33) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8525   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files        1453     1453           
  Lines      125891   125887    -4     
=======================================
+ Hits       118242   118243    +1     
+ Misses       7649     7644    -5     
Impacted Files Coverage Δ
google/cloud/bigtable/rpc_retry_policy.h 97.29% <ø> (-0.08%) ⬇️
google/cloud/pubsub/retry_policy.h 100.00% <ø> (ø)
...gle/cloud/pubsublite/internal/admin_retry_traits.h 0.00% <ø> (ø)
...oud/pubsublite/internal/topic_stats_retry_traits.h 0.00% <ø> (ø)
...le/cloud/storage/internal/curl_download_request.cc 89.75% <0.00%> (-0.36%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.25% <0.00%> (+0.15%) ⬆️
...cloud/pubsub/internal/subscription_session_test.cc 98.00% <0.00%> (+0.24%) ⬆️
google/cloud/bigtable/internal/common_client.cc 97.14% <0.00%> (+1.42%) ⬆️

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 25f6b33...fa5b655. Read the comment docs.

service {
service_proto_path: "google/cloud/gkehub/v1/service.proto"
product_path: "google/cloud/gkehub"
service_proto_path: "google/cloud/gaming/v1/game_server_configs_service.proto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you also fixed some formatting / ordering problems in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR description seems to agree with you

@dbolduc dbolduc requested a review from coryan March 11, 2022 03:44
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 06a9ccf01f0312caf3c92cf86c7288c411877c8a

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc
Copy link
Member Author

dbolduc commented Mar 11, 2022

kDeadlineExceeded is now treated as a permanent error in all of our libraries, in accordance with what we decided in our team meeting.

@coryan - I dismissed your review. I want you to check that the change to storage makes sense and that CURLE_OPERATION_TIMEDOUT should not be retried.

When I ran asan-pr locally, I had this flake (maybe related to #8385):

[ RUN      ] ObjectMediaIntegrationTest.StreamingReadTimeoutContinues
google/cloud/storage/tests/object_media_integration_test.cc:667: Failure
Value of: stream.status()
Expected: code is equal to OK and message is anything
  Actual: DEADLINE_EXCEEDED: Permanent error in Read(): PerformWork() - CURL error [28]=Timeout was reached (of type google::cloud::v1_39_0::Status), with a code that isn't equal to OK, but a message that is anything

That didn't make me feel great about this change...

@dbolduc
Copy link
Member Author

dbolduc commented Mar 11, 2022

Ok, that is at least 4 flakes on this build. I am reverting the storage/retry_policy.h change...

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: fa5b65509a2fda6883d892df2f80fc6541d5056b

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc dbolduc marked this pull request as ready for review March 12, 2022 00:53
@dbolduc dbolduc requested a review from a team as a code owner March 12, 2022 00:53
@dbolduc dbolduc merged commit ff42afc into googleapis:main Mar 12, 2022
@dbolduc dbolduc deleted the dont-retry-on-deadline-exceeded branch March 12, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants