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.

Sign up for GitHub

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

Jump to bottom

Merge connection options into client options, before defaulting the client options. #8054

Closed
dbolduc opened this issue Jan 21, 2022 · 0 comments · Fixed by #8064 or #8158
Closed

Merge connection options into client options, before defaulting the client options. #8054

dbolduc opened this issue Jan 21, 2022 · 0 comments · Fixed by #8064 or #8158
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@dbolduc
Copy link
Member

dbolduc commented Jan 21, 2022

In generated libraries, policy options passed to MakeConnection(Options options) are overwritten by the defaults in the Client.

See: (internal) "design" doc describing the problem, and the proposed solution.
@dbolduc dbolduc added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jan 21, 2022
devbww added a commit to devbww/google-cloud-cpp that referenced this issue Jan 22, 2022
@devbww
fix(spanner): apply policy options in (generated) client ctor
3feb676 
Apply any retry/backoff/polling policies in the client options
instead of the connection options.  This makes more logical
sense (particularly so when we consider per-operation options),
and is actually required until we fix googleapis#8054.

Fixes googleapis#8037.
@devbww devbww mentioned this issue Jan 22, 2022
Merged
devbww added a commit that referenced this issue Jan 22, 2022
@devbww
fix(spanner): apply policy options in (generated) client ctor (#8064)
4448e09 
Apply any retry/backoff/polling policies in the client options
instead of the connection options.  This makes more logical
sense (particularly so when we consider per-operation options),
and is actually required until we fix #8054.

Fixes #8037.
@devbww devbww reopened this Jan 22, 2022
devbww added a commit to devbww/google-cloud-cpp that referenced this issue Jan 29, 2022
@devbww
feat(generator): merge connection options into client options
3a30415 
Store the Connection options so that they may be made available to
merge into the Client options, and then use them to implement the
four polling-policy data elements.  (This is the generator version
of googleapis#8090.)

Rules of thumb for `Options` values:
- `MergeOptions()` LHS should not include the service defaults
- `MergeOptions()` RHS should include service defaults
- An `OptionsSpan` should include the service defaults
- stored `Options` (data members) should include the service defaults

Code can then normally just look at the `CurrentOptions()`, knowing
that they will include the service defaults.  For example, the policy
options will be available.  A special exception to this, however, is
when testing the connection layer in isolation (without a client), as
no one has created an `OptionsSpan` in that case.  We could add an
`OptionsSpan` to the connection-layer calls, but it would be a no-op
in practice.  So, tests need to ensure required options are available.

Fixes googleapis#8054.
@devbww devbww mentioned this issue Jan 29, 2022
Merged
devbww added a commit that referenced this issue Jan 29, 2022
@devbww
feat(generator): merge connection options into client options (#8158)
9207865 
Store the Connection options so that they may be made available to
merge into the Client options, and then use them to implement the
four polling-policy data elements.  (This is the generator version
of #8090.)

Rules of thumb for `Options` values:
- `MergeOptions()` LHS should not include the service defaults
- `MergeOptions()` RHS should include service defaults
- An `OptionsSpan` should include the service defaults
- stored `Options` (data members) should include the service defaults

Code can then normally just look at the `CurrentOptions()`, knowing
that they will include the service defaults.  For example, the policy
options will be available.  A special exception to this, however, is
when testing the connection layer in isolation (without a client), as
no one has created an `OptionsSpan` in that case.  We could add an
`OptionsSpan` to the connection-layer calls, but it would be a no-op
in practice.  So, tests need to ensure required options are available.

Fixes #8054.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
2 participants
@devbww @dbolduc