feat: Async consistency polling harness #1142
Conversation
|
|
||
| This should not be used by the user to wait until the `check_consistency` call finishes; | ||
| use the :meth:`result <google.api_core.future.async_future.AsyncFuture.result>` method of | ||
| this class instead. |
There was a problem hiding this comment.
This should not be used by the user to wait until the
check_consistencycall finishes;
use the :meth:result <google.api_core.future.async_future.AsyncFuture.result>method of
this class instead.
Is this something you added? Whay happens if the user uses this directly?
There was a problem hiding this comment.
I'm still wondering about this. It doesn't matter so much if this is internal, but I'm wondering why it needs this warning
There was a problem hiding this comment.
Ahh yeah, this is from when it wasn't internal. I think I can delete this part of the docstring now because the underscore prefix should convey this, and the fact that they're not going to use this directly.
I think my justification for this is that from the documentation perspective, it looks like done should do something for the user, and it does, but it's for the loop of using result, which indirectly calls done.
google/cloud/bigtable/admin_v2/overlay/services/bigtable_table_admin/async_client.py
Show resolved
Hide resolved
| # Wait for the table to become consistent | ||
| print("Waiting for operation to complete...") | ||
|
|
||
| response = future.result() |
There was a problem hiding this comment.
this needs an await. And the sample needs an async
| retry: OptionalRetry = gapic_v1.method.DEFAULT, | ||
| timeout: Union[float, object] = gapic_v1.method.DEFAULT, | ||
| metadata: Sequence[Tuple[str, Union[str, bytes]]] = (), | ||
| ) -> async_consistency.AsyncCheckConsistencyPollingFuture: |
There was a problem hiding this comment.
I wonder if we want to return this custom Future directly. We could encapsulate it and use pure asyncio-native coroutines instead
That would be more idiomatic, but wouldn't be as consistent between sync/async. Do you think users will do anything with this future other than call .result()?
There was a problem hiding this comment.
They could call add_done_callback but that's not documented too well. Should we do the awaiting, aka poll the call in the function instead of returning the polling harness to the user?
0983f41 to
36c2471
Compare
| try: | ||
| OptionalRetry = Union[retries.Retry, gapic_v1.method._MethodDefault, None] | ||
| except AttributeError: # pragma: NO COVER | ||
| OptionalRetry = Union[retries.Retry, object, None] # type: ignore |
There was a problem hiding this comment.
This should be AsyncRetry for the async client
| transport=transport, | ||
| client_options=client_options, | ||
| client_info=client_info, | ||
| ) |
There was a problem hiding this comment.
Is this doing anything different than the base client?
There was a problem hiding this comment.
The only thing it's doing differently is using a different default option for the client_info parameter (see lines 61-62), to provide a different version in DEFAULT_CLIENT_INFO
|
|
||
| This should not be used by the user to wait until the `check_consistency` call finishes; | ||
| use the :meth:`result <google.api_core.future.async_future.AsyncFuture.result>` method of | ||
| this class instead. |
There was a problem hiding this comment.
I'm still wondering about this. It doesn't matter so much if this is internal, but I'm wondering why it needs this warning
This PR implements both the async client's
wait_for_consistencyand__init__methods, as well as the async polling harness class.