Support forwarding arbitrary client kwargs to spanner.Client - #196
oortega15 merged 10 commits into
Conversation
Sorry, something went wrong.
Allow passing disable_builtin_metrics to SpannerConnection and connect helper functions to be forwarded to google.cloud.spanner.Client.
|
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. |
Sorry, something went wrong.
| create_ddl: Optional[Iterable[str]] = None) -> SpannerAdminApi: | ||
| create_ddl: Optional[Iterable[str]] = None, | ||
| *, | ||
| client_options: Union[api_client_options.ClientOptions, |
There was a problem hiding this comment.
Why are we adding these 2 fields in?
If the goal is to just to support disable_builtin_metrics, then this PR should not be adding this in.
If the goal is to support all fields that the Spanner client supports, then the list is not exhaustive, see: https://docs.cloud.google.com/python/docs/reference/spanner/latest/google.cloud.spanner_v1.client. There's several other fields that were not added.
I'm not sure what the best way to write this would be, we could either:
- Declare every field and pass it in. This has issue that anytime Spanner updates their client, we also need to update our wrapper.
- Use kwargs. This allows us to support new fields without needing a PR to update but has disadvantage of the IDE to pop up the available arguments for people who are depending on this library.
Personally, I'm fine with us just adding the disable_builtin_metrics but a lot of this depends on the maintenance status of this library.
Sorry, something went wrong.
There was a problem hiding this comment.
Great point! Using **client_kwargs makes much more sense so the library remains future-proof and doesn't require a new PR whenever google-cloud-spanner adds or modifies client initialization arguments. I have updated the implementation to use **client_kwargs in SpannerConnection, spanner_orm.connect(), and spanner_orm.admin.connect(), forwarding them directly to spanner.Client. This supports disable_builtin_metrics (and any other client options) seamlessly.
Sorry, something went wrong.
…ssing arbitrary keyword arguments (**client_kwargs) through SpannerConnection, spanner_orm.connect(), and spanner_orm.admin.connect() to be forwarded directly to google.cloud.spanner.Client. This makes the ORM flexible and future-proof, allowing callers to configure options like disable_builtin_metrics, route_to_leader_enabled, etc.
…ssing arbitrary keyword arguments (**client_kwargs) through SpannerConnection, spanner_orm.connect(), and spanner_orm.admin.connect() to be forwarded directly to google.cloud.spanner.Client.
|
CC: @dgorelik since I haven't reviewed code from this repo. Can you PTAL as well? |
Sorry, something went wrong.
dgorelik
left a comment
There was a problem hiding this comment.
2 notes:
- the PR title/description still refer to disable_builtin_metrics while I believe this change is more general
- i would remove the jetski/gemini output re: changes/tests in the PR description, it's pretty redundant IMO
Sorry, something went wrong.
I have fixed these notes :D |
Sorry, something went wrong.
Summary
Allows callers to pass arbitrary keyword arguments (**client_kwargs) through SpannerConnection, spanner_orm.connect(), and spanner_orm.admin.connect() to be forwarded directly to google.cloud.spanner.Client.
Motivation
Makes the client connection configurable and future-proof so callers can pass options such as disable_builtin_metrics (useful for silencing client telemetry export noise in recent google-cloud-spanner versions), route_to_leader_enabled, etc., without requiring ORM updates whenever new client parameters are introduced.
Testing