feat: replace internal dictionaries with protos in gapic calls #875
Conversation
product-auto-label
bot
added
size: m
product-auto-label
bot
added
size: l
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| @@ -36,6 +39,12 @@ class Mutation(ABC): | |||
| def _to_dict(self) -> dict[str, Any]: | |||
| raise NotImplementedError | |||
|
|
|||
| def _to_pb(self) -> data_pb.Mutation: | |||
There was a problem hiding this comment.
Do we need to double store all of the attributes? Can't we store all of the attributes in the proto directly?
Also can we drop _to_dict altogether?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also can we drop _to_dict altogether?
I think so. We do use row._to_dict in the test proxy, but I don't think we'd need it for mutations
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| @@ -104,7 +116,7 @@ def __init__( | |||
| self.timeout_generator = _attempt_timeout_generator( | |||
| attempt_timeout, operation_timeout | |||
| ) | |||
| self.mutations = mutation_entries | |||
| self.mutations = [_EntryWithProto(m, m._to_pb()) for m in mutation_entries] | |||
There was a problem hiding this comment.
If we make the mutation classes immutable, we can simplify this
There was a problem hiding this comment.
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)
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
Sure, opened a new issue for this here: #887 |
daniel-sanche
merged commit 3ac80a9
into
googleapis:experimental_v3
* feat: add new v3.0.0 API skeleton (#745) * feat: improve rows filters (#751) * feat: read rows query model class (#752) * feat: implement row and cell model classes (#753) * feat: add pooled grpc transport (#748) * feat: implement read_rows (#762) * feat: implement mutate rows (#769) * feat: literal value filter (#767) * feat: row_exists and read_row (#778) * feat: read_modify_write and check_and_mutate_row (#780) * feat: sharded read rows (#766) * feat: ping and warm with metadata (#810) * feat: mutate rows batching (#770) * chore: restructure module paths (#816) * feat: improve timeout structure (#819) * fix: api errors apply to all bulk mutations * chore: reduce public api surface (#820) * feat: improve error group tracebacks on < py11 (#825) * feat: optimize read_rows (#852) * chore: add user agent suffix (#842) * feat: optimize retries (#854) * feat: add test proxy (#836) * chore(tests): add conformance tests to CI for v3 (#870) * chore(tests): turn off fast fail for conformance tets (#882) * feat: add TABLE_DEFAULTS enum for table method arguments (#880) * fix: pass None for retry in gapic calls (#881) * feat: replace internal dictionaries with protos in gapic calls (#875) * chore: optimize gapic calls (#863) * feat: expose retryable error codes to users (#879) * chore: update api_core submodule (#897) * chore: merge main into experimental_v3 (#900) * chore: pin conformance tests to v0.0.2 (#903) * fix: bulk mutation eventual success (#909) --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Replace dicts with protos in the following rpc calls:
This will help us catch errors with easier type checking, and should have minor performance benefits
Fixes #779