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

feat: merge main branch into v1 #473

Closed
wants to merge 127 commits into from
Closed

feat: merge main branch into v1 #473

wants to merge 127 commits into from

Conversation

parthea
Copy link
Collaborator

@parthea parthea commented Dec 1, 2022

BEGIN_COMMIT_OVERRIDE
feat: merge main branch into v1
feat: Allow representing enums with their unqualified symbolic names in headers
fix: Major refactoring of Polling, Retry and Timeout logic
END_COMMIT_OVERRIDE

Including notes from PR #462

This is in response to https://freeman.vc/notes/aws-vs-gcp-reliability-is-wildly-different, which triggered an investigation of the whole Polling/Retry/Timeout behavior in Python GAPIC clients and revealed many fundamental >flaws in its implementation.

To properly describe the refactoring in this PR we need to stick to a rigorous terminology, as vague definitions of retries, timeouts, polling and related concepts seems to be the main source of the present bugs and overall >confusion among both groups: users of the library and creators of the library. Please check the updated (in this PR) documentation of the google.api_core.retry.Retry class and the google.api_core.future.polling.Polling.result>() method for the proper definitions and context.

Note, the overall semantics around Polling, Retry and Timeout remains quite confusing even after refactoring (although it is now more or less rigorously defined), but it was as clean as I could make it while still maintaining >backward compatibility of the whole library.

The quick summary of the changes in this PR:

  1. Properly define and fix the application of Deadline and Timeout concepts. Please check the updated documentation for the google.api_core.retry.Retry class for the actual definitions. Originally the deadline has been used >to represent timeouts conflating the two concepts. As result this PR replaces deadline arguments with timeout ones in as backward-compatible manner as possible (i.e. backward compatible in all practical applications).

  2. Properly define RPC Timeout, Retry Timeout and Polling Timeout and how a generic Timeout concept (aka Logical Timeout) is mapped to one of those depending on the context. Please check google.api_core.retry.Retry class >documentation for details.

  3. Properly define and fix the application of Retry and Polling concepts. Please check the updated documentation for google.api_core.future.polling.PollingFuture.result() for details.

  4. Separate retry and polling configurations for Polling future, as these are two different concepts (although both operating on Retry class). Originally both retry and polling configurations were controlled by a single >retry parameter, merging configuration regarding how "rpc error responses" and how "operation not completed" responses are supposed to be handled.

  5. For the following config properties - Retry (including Retry Timeout), Polling (including Polling Timeout) and RPC Timeout - fix and properly define how each of the above properties gets configured and which config >gets precedence in case of a conflict (check PollingFuture.result() method documentation for details). Each of those properties can be specified as follows: directly provided by the user for each call, specified during gapic >generation time from config values in grpc_service_config.json file (for Retry and RPC Timeout) and gapic.yaml file (for Polling Timeout), or be provided as a hard-coded basic default values in python-api-core library >itself. This alo includes fixing the per-call polling config propagation logic (the polling/retry configs supplied to PollingFuture.result() used to be ignored for actual call).

  6. Deprecate ExponentialTimeout, ConstantTimeout and related logic as those are outdated concepts and are not consistent with the other GAPIC Languages. Replace it with TimeToDeadlineTimeout to be consistent with how the >rest of the languages do it.

  7. Deprecate google.api_core.operations_v1.config as it is an outdated concept and self-inconsistent (as all gapic clients provide configuraiton in code). The configs are directly provided in code instead.

  8. Switch randomized delay calculation from delay being treated as expected value for randomized_delay to delay being treated as maximum value for randomized_delay (i.e. the new expected valud for randomized_delay is >delay / 2). See the exponential_sleep_generator() method implementation for details. This is needed to make Python implementation of retries and polling exponential backoff consistent with the rest of GAPIC languages. Also >fix the uncontrollable growth of delay value (since it is a subject of exponential growth, the delay value was quickly reaching "infinity" value, and the whole thing was not failing simply due to python being a very >forgiving language which forgives multiplying "infinity" by a number (inf * number = inf) binstead of simply overflowing to a (most likely) negative number). Also essentially rollback the https://github.com/googleapis/>python-api-core/commit/52f12af027568ddeae7fb6b1e3d2111cce1bac08 change, since that is inconsistent with the other languages and damages uniform distibution of retry delays artificially shifting their concentration towards the >end of timeout.

  9. Fix url construction in OperationsRestTransport. Without this fix the polling logic for REST transport was completely broken (is not affecting Compute client, as that one has custom LRO).

  10. Last but not least: change the default values for Polling logic to be the following: initial=1.0 (same as before), maximum=20.0 (was 60), multiplier=1.5 (was 2.0), timeout=900 (was 120, but due to timeout >resolution logic was actually None (i.e. infinity)). This, in conjunction with changed calculation of randomized delay (i.e. its expected value now being delay / 2) overall makes polling logic much less aggressive in terms of >increasing delays between each polling iteration, making LRO return much earlier for users on average, but still keeping a healthy balance between strain put on both client and server by polling and responsiveness of LROs for >user.

*The design doc summarising all the changes and reasons for them is in progress.

In addition to the timeout/retry fixes, this PR has some other non-related technical fixes:

  • Fix and improve code coverage (and explicitly disables lines which are not supposed to be covered)
  • Define Python 3.10 as the default python version used in CI.
  • Upgrade to Sphinx 4.2.0, as it is the one supporting Python 3.10

busunkim96 and others added 30 commits August 3, 2021 11:47
Drop 'six' module

Drop 'u"' prefixes for text

Remove other Python 2.7 workarounds

Drop use of 'pytz'

Dxpand range to allow 'google-auth' 2.x versions

Remove 'general_helpers.wraps': except for a backward-compatibility
import, 'functools.wraps' does everything wee need on Python >= 3.6.

Remove 'packaging' dependency

Release-As: 2.0.0b1

Closes #74.

Closes #215.
Source-Link: googleapis/synthtool@7e1f6da
Post-Processor: gcr.io/repo-automation-bots/owlbot-python:latest@sha256:a1a891041baa4ffbe1a809ac1b8b9b4a71887293c9101c88e8e255943c5aec2d
Rely on the pins in 'setup.py' as the Source of Truth.

See #234 (review)
Source-Link: googleapis/synthtool@facee4c
Post-Processor: gcr.io/repo-automation-bots/owlbot-python:latest@sha256:9743664022bd63a8084be67f144898314c7ca12f0a03e422ac17c733c129d803

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
chore: removing owlbot directives for conversion to main
* feat: add grpc transcoding + tests

* 🦉 Updates from OwlBot

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

* chore: tweak for clarity / idiomatic usage

* chore: attempt to appease Sphinx

* feat: add grpc transcoding + tests

* Add functions to properly handle subfields

* Add unit tests for get_field and delete_field.

* Add function docstrings and incorporate correct native dict functions.

* Add function docstrings and incorporate correct native dict functions.

* Increase code coverage

* Increase code coverage

* Increase code coverage

* Reformat files

Co-authored-by: Yonatan Getahun <yonmg@google.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
…275)

Co-authored-by: Kenneth Bandes <kbandes@google.com>
chore: relocate owl bot post processor
…282)

* chore: add default_version and codeowner_team to .repo-metadata.json

* Assign @googleapis/actools-python as codeowner
…le (#283)

Source-Link: googleapis/synthtool@a7ed11e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:6e7328583be8edd3ba8f35311c76a1ecbc823010279ccb6ab46b7a76e25eafcc

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Fix new deprecation warning for 'threading.Condition.notifyAll'.
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Source-Link: googleapis/synthtool@c6e69c4
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:58f73ba196b5414782605236dd0712a73541b44ff2ff4d3a36ec41092dd6fa5b

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
gcf-owl-bot bot and others added 20 commits September 1, 2022 18:28
…430)

Source-Link: googleapis/synthtool@fdba3ed
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:1f0dbd02745fb7cf255563dab5968345989308544e52b7f460deadd5e78e63b0
docs: add a note that grpcio-gcp is only supported in environments with protobuf < 4.x.x
docs: raise DeprecationWarning when 'grpcio-gcp' is used
fix(deps): require protobuf >= 3.20.1
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Source-Link: googleapis/synthtool@703554a
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:94961fdc5c9ca6d13530a6a414a49d2f607203168215d074cdb0a1df9ec31c0b

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
* feat: add 'strict' to flatten_query_params to lower-case bools

* pylint

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Source-Link: googleapis/synthtool@56da63e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:993a058718e84a82fda04c3177e58f0a43281a996c7c395e0a56ccc4d6d210d7
* fix: Improve transcodding error message

This fixes #441 and #440. Also, with this change we stop outputting the whole request message in transcodding erro message to prevent leaking any confidential information from a request message in a form of an error in a log message.

* reformat code with black
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
* chore: exclude requirements.txt file from renovate-bot

Source-Link: googleapis/synthtool@f58d313
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:7a40313731a7cb1454eef6b33d3446ebb121836738dc3ab3d2d3ded5268c35b6

* update constraints files

* fix(deps): require protobuf 3.20.2

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
* fix(deps): allow protobuf 3.19.5

* explicitly exclude protobuf 4.21.0

* chore: use a compatible version of protobuf for testing
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
* fix: Major refactoring and fix for Polling, Retry and Timeout logic

This is in response to https://freeman.vc/notes/aws-vs-gcp-reliability-is-wildly-different, which triggered an investigation of the whole Polling/Retry/Timeout behavior in Python GAPIC clients and revealed many fundamental flaws in its implementaiton.

To properly describe the refactoring this PR does we need to stick to a rigorous terminology, as vague definitions of retries, timeouts, polling and related concepts seems to be the main source of the present bugs and overal confusion among both groups: users of the library and creators of the library. Please check the documentation of the `google.api_core.retry.Retry` class the `google.api_core.future.polling.Polling.result()` method for the proper definitions and context.

Note, the overall semantics around Polling, Retry and Timeout remains quite confusing even after refactoring (although it is now more or less rigorously defined), but it was clean as I could make it while still maintaining backward compatibility of the whole library.

The quick summary of the changes in this PR:

1) Properly define and fix the application of Deadline and Timeout concepts. Please check the updated documentation for the `google.api_core.retry.Retry` class for the actual definitions. Originally the `deadline` has been used to represent timeouts conflating the two concepts. As result this PR replaces `deadline` arguments with `timeout` ones in as backward-compatible manner as possible (i.e. backward compatible in all practical applications).

2) Properly define RPC Timeout, Retry Timeout and Pollint Timeout and how a generic Timeout concept (aka Logical Timeout) is mapped to one of those depending on the context. Please check `google.api_core.retry.Retry` class documentation for details.

3) Properly define and fix the application of Retry and Polling concepts. Please check the updated documentation for `google.api_core.future.polling.PollingFuture.result()` for details.

4) Separate `retry` and `polling` configurations for Polling future, as these are two different concepts (although both operating on `Retry` class). Originally both retry and polling configurations were controlled by a single `retry` parameter, merging configuration regarding how "rpc error responses" and how "operation not completed" responses are supposed to be handled.

5) For the following config properties - `Retry (including `Retry Timeout`), `Polling` (including `Polling Timeout`) and `RPC Timeout` - fix and properly define how each of the above properties gets configured and which config gets precedence in case of a conflict (check `PollingFuture.result()` method documentation for details). Each of those properties can be specified as follows: directly provided by the user for each call, specified during gapic generation time from config values in `grpc_service_config.json` file (for Retry and RPC Timeout) and `gapic.yaml` file (for Polling), or be provided as a hard-coded basic default values in python-api-core library itself.

6) Fix the per-call polling config propagation logic (the polling/retry configs supplied to `PollingFuture.result()` used to be ignored for actual call).

7) Deprecate the usage of `deadline` terminology in the whole library and backward-compatibly replace it with timeout. This is essential as what has been called "deadline" in this library was actually "timeout" as it is defined in `google.api_core.retry.Retry` class documentation.

8) Deprecate `ExponentialTimeout`, `ConstantTimeout` and related logic as those are outdated concepts and are not consistent with the other GAPIC Languages. Replace it with `TimeToDeadlineTimeout` to be consistent with how the rest of the languages do it.

9) Deprecate `google.api_core.operations_v1.config` as it is an outdated concept and self-inconsistent (as all gapic clients provide configuraiton in code). The configs are directly provided in code instead.

10) Switch randomized delay calculation from `delay` being treated as expected value for randomized_delay to `delay` being treated as maximum value for `randomized_delay` (i.e. the new expected valud for `randomized_delay` is `delay / 2`). See the `exponential_sleep_generator()` method implementation for details. This is needed to make Python implementation of retries and polling exponential backoff consistent with the rest of GAPIC languages. Also fix the uncontrollable growth of `delay` value (since it is a subject of exponential growth, the `delay` value was quickly reaching "infinity" value, and the whole thing was not failing simply due to python being a very forgiving language which forgives multiplying "infinity" by a number (`inf * number = inf`) binstead of simply overflowing to a (most likely) negative number).

11) Fix url construction in `OperationsRestTransport`. Without this fix the polling logic for REST transport was completely broken (is not affecting Compute client, as that one has custom LRO).

12) Las but not least: change the default values for Polling logic to be the following: `initial=1.0` (same as before), `maximum=20.0` (was `60`), `multiplier=1.5` (was `2.0`), `timeout=900` (was `120`, but due to timeout resolution logic was actually None (i.e. infinity)). This, in conjunction with changed calculation of randomized delay (i.e. its expected value now being  `delay / 2`) overall makes polling logic much less aggressive in terms of increasing delays between each polling iteration, making LRO return much earlier for users on average, but still keeping a healthy balance between strain put on both client and server by polling and responsiveness of LROs for user.

*The design doc summarising all the changes and reasons for them is in progress.

* fix ci failures (mainly sphinx errors)

* remove unused code

* fix typo

* Pin pytest version to <7.2.0

* reformat code

* address pr feedback

* address PR feedback

* address pr feedback

* Update google/api_core/future/polling.py

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

* Apply documentation suggestions from code review

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

* Address PR feedback

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>
* bump google-auth version to 2.13.0

* chore: set minimum to 2.14.1 instead

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
…pprove] (#464)

* chore(python): update dependencies in .kokoro/requirements.txt

Source-Link: googleapis/synthtool@e3a1277
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:452901c74a22f9b9a3bd02bce780b8e8805c97270d424684bff809ce5be8c2a2

* 🦉 Updates from OwlBot post-processor

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

* exclude templated lint github action

* 🦉 Updates from OwlBot post-processor

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

* fix typo

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
* feat: add support for Python 3.11

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

* restore check for unit-3.10_wo_grpc

* restore check for unit_wo_grpc-3.10

* restore 3.10 test

* require grpcio 1.49.1 with python 3.11

* lint

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
…in headers (#465)

* feat: Allow non-fully-qualified enums in routing headers

* Rename s/fully_qualified_enums/qualified_enums/g for correctness

* chore: minor tweaks

* chore: Temporary workaround for pytest in noxfile.

* Fix import order

* bring coverage to 100%

* lint

* 🦉 Updates from OwlBot post-processor

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

* remove replacement in owlbot.py causing lint failure

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
* chore(python): drop flake8-import-order in samples noxfile

Source-Link: googleapis/synthtool@6ed3a83
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3abfa0f1886adaf0b83f07cb117b24a639ea1cb9cffe56d43280b977033563eb

* drop flake8-import-order

* lint

* use python 3.9 for docs

* resolve mypy error

* update python version for lint

* fix lint

* fix lint

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
@parthea parthea requested review from a team as code owners December 1, 2022 18:44
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 1, 2022
@google-cla
Copy link

google-cla bot commented Dec 1, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@parthea parthea closed this Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.