feat: add generated sync client #1017
Conversation
This reverts commit 37b4833.
daniel-sanche
changed the title
| @@ -305,7 +305,7 @@ async def append(self, mutation_entry: RowMutationEntry): | |||
| # TODO: return a future to track completion of this entry | |||
| if self._closed.is_set(): | |||
| raise RuntimeError("Cannot append to closed MutationsBatcher") | |||
| if isinstance(mutation_entry, Mutation): # type: ignore | |||
| if isinstance(cast(Mutation, mutation_entry), Mutation): | |||
There was a problem hiding this comment.
Why do we need cast() here?
There was a problem hiding this comment.
TL;DR: because comments aren't part of the ast, so # type: ignore doesn't make it through the generation process for the sync client. But typing.cast does the same thing, by forcing the type checker to treat it as a specific type here
For why we need to ignore the type in the first place: this is a check to raise a helpful error when the user passes in a Mutation instead of a RowMutationEntry, which could be an easy mistake to make. But the type checker doesn't like this check, since we're checking if it's a type that it knows it should never be
There was a problem hiding this comment.
Thanks for the explanation!
| gapic_client.mutate_rows, | ||
| table_name=table.table_name, | ||
| app_profile_id=table.app_profile_id, | ||
| retry=None, |
There was a problem hiding this comment.
what does this parameter do and why is it none?
There was a problem hiding this comment.
This would be the gapic-level retry logic. We set it to None because we handle retries at a higher-level, retrying self._run_attempt instead (It's wrapped by CrossSync._Sync_Impl.retry_target into self._operation a few lines down)
| self._handle_entry_error(orig_idx, entry_error) | ||
| elif orig_idx in self.errors: | ||
| del self.errors[orig_idx] | ||
| del active_request_indices[result.index] |
There was a problem hiding this comment.
this modifies the active request list, will line 149 orig_idx = active_request_indices[result.index] still be correct after a deletion? would result.index be > active_request_indices at some point?
There was a problem hiding this comment.
active_request_indices is a map, not a list. It maps indexes between the original overall request, and the latest attempt (which may be for a small subset of the original mutations)
We should never encounter a result.index that isn't in active_request_indices, because they are both derived from remaining_indices
| @@ -0,0 +1,828 @@ | |||
| # Copyright 2023 Google LLC | |||
There was a problem hiding this comment.
this should be 2024 in all the files, wondering if it's because in async it's 2023..
There was a problem hiding this comment.
Yeah, I had the code generator copy over all header comments from the async files. Using a different year for the generated files would be tricky. And I thought since 90% of the files are the same, it makes sense to keep the same publication year, since it's closer to a copy than an original file. But maybe we should use a year range instead?
There was a problem hiding this comment.
Can we change all 2023 to 2024 for now?
This PR adds the sync version of the new data client to the codebase:
_sync_autogendirectoriesverification tests moved to new PR, to keep this mostly generated code: #1046