feat: use full jitter exp backoff policy in the generator #11748

alevenberg · 2023-05-30T18:49:29Z

Fixes issue #8755

This change is

coryan · 2023-05-30T18:55:51Z

generator/internal/option_defaults_generator.cc

@@ -153,8 +153,8 @@ Status OptionDefaultsGenerator::GenerateCc() {
    "  }\n"
    "  if (!options.has<$product_namespace$::$service_name$BackoffPolicyOption>()) {\n"
    "    options.set<$product_namespace$::$service_name$BackoffPolicyOption>(\n"
-    "        ExponentialBackoffPolicy(std::chrono::seconds(1),\n"
-    "            std::chrono::minutes(5), kBackoffScaling).clone());\n"
+    "        ExponentialBackoffPolicy(std::chrono::seconds(0), std::chrono::seconds(1), \n"


nit: maybe avoid generating a trailing space? All we will do is erase it in one of the formatters.

dbolduc

Reviewed 3 of 233 files at r1, all commit messages.
Reviewable status: 3 of 233 files reviewed, 2 unresolved discussions (waiting on @alevenberg and @coryan)

generator/internal/option_defaults_generator.cc line 166 at r1 (raw file):

    "            $product_namespace$::$service_name$BackoffPolicyOption::Type>(\n"
    "            options.get<$product_namespace$::$retry_policy_name$Option>()->clone(),\n"
    "            options.get<$product_namespace$::$service_name$BackoffPolicyOption>()->clone())\n"

We should change this back to the old value.

alevenberg

Reviewable status: 3 of 233 files reviewed, 1 unresolved discussion (waiting on @coryan and @dbolduc)

generator/internal/option_defaults_generator.cc line 166 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

We should change this back to the old value.

What do you mean?

dbolduc

Reviewable status: 3 of 233 files reviewed, 1 unresolved discussion (waiting on @alevenberg and @coryan)

generator/internal/option_defaults_generator.cc line 166 at r1 (raw file):

Previously, alevenberg (Anna Levenberg) wrote…

What do you mean?

We should use ExponentialBackoffPolicy(std::chrono::seconds(1), std::chrono::minutes(5), kBackoffScaling) instead of cloning the *BackoffPolicyOption

(Because full jitter is good for individual retries to address the thundering herd problem. But it is not best for polling LROs. We do not want to be waking up background threads after 0 seconds to check the status of the operation.)

codecov · 2023-05-30T20:43:13Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (403ca83) 93.77% compared to head (0108ee2) 93.77%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11748   +/-   ##
=======================================
  Coverage   93.77%   93.77%           
=======================================
  Files        1831     1831           
  Lines      165125   165157   +32     
=======================================
+ Hits       154839   154871   +32     
  Misses      10286    10286

Impacted Files	Coverage Δ
generator/internal/option_defaults_generator.cc	`73.91% <ø> (ø)`
...v1/internal/golden_kitchen_sink_option_defaults.cc	`100.00% <100.00%> (ø)`
...en/v1/internal/golden_rest_only_option_defaults.cc	`100.00% <100.00%> (ø)`
.../v1/internal/golden_thing_admin_option_defaults.cc	`100.00% <100.00%> (ø)`
.../tests/golden_kitchen_sink_option_defaults_test.cc	`100.00% <100.00%> (ø)`
...s/tests/golden_thing_admin_option_defaults_test.cc	`100.00% <100.00%> (ø)`
...le/cloud/pubsub/internal/schema_option_defaults.cc	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

alevenberg

Reviewable status: 2 of 233 files reviewed, 1 unresolved discussion (waiting on @coryan and @dbolduc)

generator/internal/option_defaults_generator.cc line 166 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

We should use ExponentialBackoffPolicy(std::chrono::seconds(1), std::chrono::minutes(5), kBackoffScaling) instead of cloning the *BackoffPolicyOption

(Because full jitter is good for individual retries to address the thundering herd problem. But it is not best for polling LROs. We do not want to be waking up background threads after 0 seconds to check the status of the operation.)

Done.

dbolduc

It might be worth adding tests to ...

... to verify that the retry / backoff / polling policy options have values. (We wouldn't need to check what their value is.)

generator/internal/option_defaults_generator.cc

* add golden kitchen sink options default to test for retry, backoff, and idempotency * add golden thing admin tests for backoff, retry, and polling policy

dbolduc

The inline namespace thing needs to get fixed, but I trust you can make the change.

generator/integration_tests/tests/golden_kitchen_sink_option_defaults_test.cc

generator/integration_tests/tests/golden_thing_admin_option_defaults_test.cc

…ine namespaces

alevenberg

Done

Reviewable status: 2 of 235 files reviewed, 5 unresolved discussions (waiting on @coryan and @dbolduc)

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

This is the only line we want to change.

Done

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

git push ?

Whoops. Done

generator/integration_tests/tests/golden_kitchen_sink_option_defaults_test.cc line 32 at r3 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

we should not be using the inline namespace, v2_11. Inline namespaces are only used by libraries/applications that need to use two versions of google-cloud-cpp at the same time. (which is hopefully rare).
using ::google::cloud::golden_v1::GoldenKitchenSinkBackoffPolicyOption;

Done

generator/integration_tests/tests/golden_kitchen_sink_option_defaults_test.cc line 91 at r3 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

nit: here and below, we always say:
EXPECT_TRUE(updated_options.has<GoldenKitchenSinkRetryPolicyOption>());

Done

generator/integration_tests/tests/golden_thing_admin_option_defaults_test.cc line 29 at r3 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

these are right 👍

Done.

alevenberg

Dismissed @dbolduc from 5 discussions.
Reviewable status: 2 of 235 files reviewed, all discussions resolved (waiting on @coryan and @dbolduc)

coryan

Reviewed 122 of 233 files at r1, 111 of 113 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alevenberg)

alevenberg requested a review from a team as a code owner May 30, 2023 18:49

coryan reviewed May 30, 2023

View reviewed changes

alevenberg added 2 commits May 30, 2023 19:40

feat: use full jitter in generator

46df77a

generator changes

1f24eec

alevenberg force-pushed the bug-8755 branch from 5cbb26f to 1f24eec Compare May 30, 2023 19:44

coryan approved these changes May 30, 2023

View reviewed changes

dbolduc requested changes May 30, 2023

View reviewed changes

alevenberg commented May 30, 2023

View reviewed changes

dbolduc reviewed May 30, 2023

View reviewed changes

alevenberg added 2 commits May 30, 2023 21:21

add backoff policy for polling (lro)

0c88dac

generator changes

4271d3d

alevenberg commented May 30, 2023

View reviewed changes

dbolduc reviewed May 30, 2023

View reviewed changes

generator/internal/option_defaults_generator.cc Show resolved Hide resolved

generator/internal/option_defaults_generator.cc Outdated Show resolved Hide resolved

alevenberg added 5 commits May 31, 2023 14:49

only replace backoff policy

bb5ce84

generator changes

79cbe18

add missing comma, clone, and closing paren

2c63ad3

generator

92c4264

add golden tests to check for policy

76ecfbb

* add golden kitchen sink options default to test for retry, backoff, and idempotency * add golden thing admin tests for backoff, retry, and polling policy

dbolduc approved these changes May 31, 2023

View reviewed changes

address comments: expect_eq(true, x) -> expect_true(x) and remove inl…

0108ee2

…ine namespaces

dbolduc approved these changes May 31, 2023

View reviewed changes

alevenberg commented May 31, 2023

View reviewed changes

coryan approved these changes May 31, 2023

View reviewed changes

alevenberg merged commit 3f504e5 into googleapis:main May 31, 2023

alevenberg deleted the bug-8755 branch June 20, 2023 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use full jitter exp backoff policy in the generator #11748

feat: use full jitter exp backoff policy in the generator #11748

alevenberg commented May 30, 2023 •

edited by coryan

Loading

coryan May 30, 2023

dbolduc left a comment

alevenberg left a comment

dbolduc left a comment

codecov bot commented May 30, 2023 •

edited

Loading

alevenberg left a comment

dbolduc left a comment

dbolduc left a comment

alevenberg left a comment

alevenberg left a comment

coryan left a comment

feat: use full jitter exp backoff policy in the generator #11748

feat: use full jitter exp backoff policy in the generator #11748

Conversation

alevenberg commented May 30, 2023 • edited by coryan Loading

coryan May 30, 2023

Choose a reason for hiding this comment

dbolduc left a comment

Choose a reason for hiding this comment

alevenberg left a comment

Choose a reason for hiding this comment

dbolduc left a comment

Choose a reason for hiding this comment

codecov bot commented May 30, 2023 • edited Loading

Codecov Report

alevenberg left a comment

Choose a reason for hiding this comment

dbolduc left a comment

Choose a reason for hiding this comment

dbolduc left a comment

Choose a reason for hiding this comment

alevenberg left a comment

Choose a reason for hiding this comment

alevenberg left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

alevenberg commented May 30, 2023 •

edited by coryan

Loading

codecov bot commented May 30, 2023 •

edited

Loading