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: Do not allow gaxOptions to be modified downstream #1379

Merged

Conversation

danieljbruce
Copy link
Contributor

@danieljbruce danieljbruce commented Jan 22, 2024

Currently, #1372 has a failing test that produces an error that says Uncaught Error: Only one of retry or retryRequestOptions may be set. This code change allows that test to pass on the gcf-owl-bot PR branch.

Here is what currently happens:

The failing test uses a mock server that aborts the stream simulating a network error. This causes the client to retry. However, on the first call the client makes, options.gaxOptions gets passed down the stack at

gaxOpts: options.gaxOptions,
and then modified so that the retry parameter of options.gaxOptions is defined having settings that match https://github.com/googleapis/nodejs-bigtable/blob/8cc17aa2acb2540b4c2b233c4245021815a7b030/src/v2/bigtable_client_config.json#L12-L20.. On the second call, options.gaxOptions is passed down the stack with the retry parameter defined this time and with the retryRequestOptions parameter attached at
retryRequestOptions,
as well. This causes the error to be thrown that says Uncaught Error: Only one of retry or retryRequestOptions may be set.

What this fix does:

With this fix a copy of options.gaxOptions is passed down the call stack instead of options.gaxOptions itself. This means when the client retries, options.gaxOptions does not contain the retry parameter in the createReadStream function as this parameter is normally filled out much further down the stack. This means the error Uncaught Error: Only one of retry or retryRequestOptions may be set is not thrown.

Additional effects:

For retries, this means options.gaxOptions?.retry?.backoffSettings will be undefined at

const backOffSettings =
options.gaxOptions?.retry?.backoffSettings ||
DEFAULT_BACKOFF_SETTINGS;
and retries will now use DEFAULT_BACKOFF_SETTINGS if backoff settings are not provided by the user. This is exactly what we want and is also the intent of the current code.

@danieljbruce danieljbruce requested review from a team as code owners January 22, 2024 21:18
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Jan 22, 2024
@danieljbruce danieljbruce merged commit 6a889ea into googleapis:owl-bot-copy Jan 22, 2024
12 of 16 checks passed
danieljbruce added a commit that referenced this pull request Jan 23, 2024
* feat: Modify ModifyColumnFamiliesRequest proto to expose ignore_warnings field

PiperOrigin-RevId: 590940407

Source-Link: googleapis/googleapis@fb027c8

Source-Link: googleapis/googleapis-gen@f0728cd
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjA3MjhjZGEyMjdiMzg4MzU4MjJjNGU1NTE5ZTU2OGNlOGQyYjVhYyJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat: Adding feature flags for routing cookie and retry info

PiperOrigin-RevId: 591912877

Source-Link: googleapis/googleapis@f6505fe

Source-Link: googleapis/googleapis-gen@7499187
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNzQ5OTE4NzQxNWY4ZDQwNWVmMGQ0NmRkNmZmNjA4YjEyNWM1M2M4ZiJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* docs: update comment for `AccountVerificationInfo.username`

PiperOrigin-RevId: 598888823

Source-Link: googleapis/googleapis@955ff0a

Source-Link: googleapis/googleapis-gen@8b0b992
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiOGIwYjk5MmNkY2EwMDQxOWNkZjQ0MzNkNmJiZTNjZjNlOTNmMWRkNCJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat: configure PHP client to generate new surface

PiperOrigin-RevId: 599228576

Source-Link: googleapis/googleapis@ef54eaf

Source-Link: googleapis/googleapis-gen@4d0d81b
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNGQwZDgxYjc4MjJlMmYzNjFkZTZkNmUwMWIzNzVjY2M5YWYzZTEwOSJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* fix: improve retry logic for streaming API calls
build: update typescript generator version to 4.3.0

The streaming API call retry logic has changed, which in some rare cases may require code changes. Please feel free to reach out to us in the issues if you experience new problems with retrying streaming calls after this update.

PiperOrigin-RevId: 599622271

Source-Link: googleapis/googleapis@6239c21

Source-Link: googleapis/googleapis-gen@da13d82
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZGExM2Q4MjIyZDNiYTMzNzM0NTAxOTk5ODY0NDU4NjQwZjE0MDVhZSJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* Make a copy of the gaxOpts parameter

Don’t modify the gaxOptions parameter in place and pass it down. This change makes it so that changes to gax options downstream will not affect gaxOptions in this layer of the call stack which is desirable and which also allows the failing test to pass.

* Revert "Make a copy of the gaxOpts parameter"

This reverts commit aa0c46b.

* Revert "Revert "Make a copy of the gaxOpts parameter"" (#1379)

This reverts commit 3cadbe0.

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Daniel Bruce <djbruce@google.com>
Co-authored-by: danieljbruce <danieljbruce@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. size: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants