Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 65
feat: replace internal dictionaries with protos in gapic calls#875
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
7aad7a0c68e410de89f9257d65efead484a5197d13dc596df5bbae6cfcfcc1dd98cf46a0c88349300ec5File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -924,22 +924,17 @@ async def mutate_row( | ||
| GoogleAPIError exceptions from any retries that failed | ||
| - GoogleAPIError: raised on non-idempotent operations that cannot be | ||
| safely retried. | ||
| - ValueError if invalid arguments are provided | ||
| """ | ||
| operation_timeout, attempt_timeout = _get_timeouts( | ||
| operation_timeout, attempt_timeout, self | ||
| ) | ||
| if isinstance(row_key, str): | ||
| row_key = row_key.encode("utf-8") | ||
| request = {"table_name": self.table_name, "row_key": row_key} | ||
| if self.app_profile_id: | ||
| request["app_profile_id"] = self.app_profile_id | ||
| if not mutations: | ||
| raise ValueError("No mutations provided") | ||
| mutations_list = mutations if isinstance(mutations, list) else [mutations] | ||
| if isinstance(mutations, Mutation): | ||
| mutations = [mutations] | ||
| request["mutations"] = [mutation._to_dict() for mutation in mutations] | ||
| if all(mutation.is_idempotent() for mutation in mutations): | ||
| if all(mutation.is_idempotent() for mutation in mutations_list): | ||
igorbernstein2 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| # mutations are all idempotent and safe to retry | ||
| predicate = retries.if_exception_type( | ||
| core_exceptions.DeadlineExceeded, | ||
| @@ -972,7 +967,13 @@ def on_error_fn(exc): | ||
| metadata = _make_metadata(self.table_name, self.app_profile_id) | ||
| # trigger rpc | ||
| await deadline_wrapped( | ||
| request, timeout=attempt_timeout, metadata=metadata, retry=None | ||
| row_key=row_key.encode("utf-8") if isinstance(row_key, str) else row_key, | ||
| mutations=[mutation._to_pb() for mutation in mutations_list], | ||
| table_name=self.table_name, | ||
| app_profile_id=self.app_profile_id, | ||
| timeout=attempt_timeout, | ||
| metadata=metadata, | ||
| retry=None, | ||
| ) | ||
| async def bulk_mutate_rows( | ||
| @@ -1009,6 +1010,7 @@ async def bulk_mutate_rows( | ||
| Raises: | ||
| - MutationsExceptionGroup if one or more mutations fails | ||
| Contains details about any failed entries in .exceptions | ||
| - ValueError if invalid arguments are provided | ||
| """ | ||
| operation_timeout, attempt_timeout = _get_timeouts( | ||
| operation_timeout, attempt_timeout, self | ||
| @@ -1065,29 +1067,24 @@ async def check_and_mutate_row( | ||
| - GoogleAPIError exceptions from grpc call | ||
| """ | ||
| operation_timeout, _ = _get_timeouts(operation_timeout, None, self) | ||
| row_key = row_key.encode("utf-8") if isinstance(row_key, str) else row_key | ||
| if true_case_mutations is not None and not isinstance( | ||
| true_case_mutations, list | ||
| ): | ||
| true_case_mutations = [true_case_mutations] | ||
| true_case_dict = [m._to_dict() for m in true_case_mutations or []] | ||
| true_case_list = [m._to_pb() for m in true_case_mutations or []] | ||
| if false_case_mutations is not None and not isinstance( | ||
| false_case_mutations, list | ||
| ): | ||
| false_case_mutations = [false_case_mutations] | ||
| false_case_dict = [m._to_dict() for m in false_case_mutations or []] | ||
| false_case_list = [m._to_pb() for m in false_case_mutations or []] | ||
| metadata = _make_metadata(self.table_name, self.app_profile_id) | ||
| result = await self.client._gapic_client.check_and_mutate_row( | ||
| request={ | ||
| "predicate_filter": predicate._to_dict() | ||
| if predicate is not None | ||
| else None, | ||
| "true_mutations": true_case_dict, | ||
| "false_mutations": false_case_dict, | ||
| "table_name": self.table_name, | ||
| "row_key": row_key, | ||
| "app_profile_id": self.app_profile_id, | ||
| }, | ||
| true_mutations=true_case_list, | ||
| false_mutations=false_case_list, | ||
| predicate_filter=predicate._to_pb() if predicate is not None else None, | ||
| row_key=row_key.encode("utf-8") if isinstance(row_key, str) else row_key, | ||
| table_name=self.table_name, | ||
| app_profile_id=self.app_profile_id, | ||
| metadata=metadata, | ||
| timeout=operation_timeout, | ||
| retry=None, | ||
| @@ -1123,25 +1120,21 @@ async def read_modify_write_row( | ||
| operation | ||
| Raises: | ||
| - GoogleAPIError exceptions from grpc call | ||
| - ValueError if invalid arguments are provided | ||
| """ | ||
| operation_timeout, _ = _get_timeouts(operation_timeout, None, self) | ||
| row_key = row_key.encode("utf-8") if isinstance(row_key, str) else row_key | ||
| if operation_timeout <= 0: | ||
| raise ValueError("operation_timeout must be greater than 0") | ||
| if rules is not None and not isinstance(rules, list): | ||
| rules = [rules] | ||
| if not rules: | ||
| raise ValueError("rules must contain at least one item") | ||
| # concert to dict representation | ||
| rules_dict = [rule._to_dict() for rule in rules] | ||
| metadata = _make_metadata(self.table_name, self.app_profile_id) | ||
| result = await self.client._gapic_client.read_modify_write_row( | ||
| request={ | ||
| "rules": rules_dict, | ||
| "table_name": self.table_name, | ||
| "row_key": row_key, | ||
| "app_profile_id": self.app_profile_id, | ||
| }, | ||
| rules=[rule._to_pb() for rule in rules], | ||
| row_key=row_key.encode("utf-8") if isinstance(row_key, str) else row_key, | ||
| table_name=self.table_name, | ||
| app_profile_id=self.app_profile_id, | ||
| metadata=metadata, | ||
| timeout=operation_timeout, | ||
| retry=None, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -19,9 +19,12 @@ | ||
| from abc import ABC, abstractmethod | ||
| from sys import getsizeof | ||
| import google.cloud.bigtable_v2.types.bigtable as types_pb | ||
| import google.cloud.bigtable_v2.types.data as data_pb | ||
| from google.cloud.bigtable.data.read_modify_write_rules import _MAX_INCREMENT_VALUE | ||
| # special value for SetCell mutation timestamps. If set, server will assign a timestamp | ||
| _SERVER_SIDE_TIMESTAMP = -1 | ||
| @@ -36,6 +39,12 @@ class Mutation(ABC): | ||
| def _to_dict(self) -> dict[str, Any]: | ||
| raise NotImplementedError | ||
| def _to_pb(self) -> data_pb.Mutation: | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to double store all of the attributes? Can't we store all of the attributes in the proto directly? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the main downside is we'd need a lot more boilerplate setters/getters (we lose the simplicity of the dataclasses). And marshaling to/from the protos is more expensive, but I guess that should'nt be too much of an issue here. I see this is how we handled it in the ReadRowsQuery class, which is more complicated than these would be. What do you think of making these immutable? That would simplify the setters/getters, and then we wouldn't have to worry about making a static copy before starting a mutate_rows operation ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so. We do use Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally the logic for converting from models to protos for the test proxy is self contained in the proxy. A side goal of test proxy was to be a rosetta stone for expressing the same concepts across every cleint impl Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I dont think you need to worry about someone mutating the model after passing it to the proxy...I dont think you need to be that defensive in your code | ||
| """ | ||
| Convert the mutation to protobuf | ||
| """ | ||
| return data_pb.Mutation(**self._to_dict()) | ||
| def is_idempotent(self) -> bool: | ||
| """ | ||
| Check if the mutation is idempotent | ||
| @@ -221,6 +230,12 @@ def _to_dict(self) -> dict[str, Any]: | ||
| "mutations": [mutation._to_dict() for mutation in self.mutations], | ||
| } | ||
| def _to_pb(self) -> types_pb.MutateRowsRequest.Entry: | ||
| return types_pb.MutateRowsRequest.Entry( | ||
| row_key=self.row_key, | ||
| mutations=[mutation._to_pb() for mutation in self.mutations], | ||
| ) | ||
| def is_idempotent(self) -> bool: | ||
| """Check if the mutation is idempotent""" | ||
| return all(mutation.is_idempotent() for mutation in self.mutations) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make the mutation classes immutable, we can simplify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think making each individual mutation (ie SetCell, DeleteCell, etc) is a great idea
Howevr I dont think you can make the collection of mutations immutable (ie RowMutationEntry)