Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: implementOperationsRestAsyncTransport to support long running operations#700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

parthea
Copy link
Collaborator

@partheaparthea commentedSep 20, 2024•
edited
Loading

Towards b/362946071

This PR depends on#701

@product-auto-labelproduct-auto-labelbot added the size: lPull request size is large. labelSep 20, 2024
@partheapartheaforce-pushed theoperations-rest-async-transport branch 7 times, most recently from21bdadd to1e108b4CompareSeptember 20, 2024 20:30
@partheaparthea changed the base branch frommain toadd-extra-for-aiohttpSeptember 20, 2024 20:30
@partheapartheaforce-pushed theoperations-rest-async-transport branch 2 times, most recently from0ee82e0 to6e51532CompareSeptember 20, 2024 20:50
@partheapartheaforce-pushed theoperations-rest-async-transport branch 2 times, most recently fromb6c1c6c toeb78445CompareSeptember 23, 2024 16:43
@partheaparthea marked this pull request as ready for reviewSeptember 25, 2024 15:06
@partheaparthea requested review froma team ascode ownersSeptember 25, 2024 15:06
@ohmayrohmayr changed the titlefeat: Add OperationsRestAsyncTransport to support long running operationsfeat: implementOperationsRestAsyncTransport to support long running operationsSep 30, 2024
Base automatically changed fromadd-extra-for-aiohttp tomainSeptember 30, 2024 20:58
@ohmayrohmayrforce-pushed theoperations-rest-async-transport branch frombf76338 tof880c72CompareSeptember 30, 2024 20:59
@ohmayrohmayr changed the base branch frommain toasync-rest-lro-support-in-coreOctober 1, 2024 19:45
from google.auth.aio import credentials as ga_credentials_async

try:
import aiohttp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do we useaiohttp in this file? A search reveals no references

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We don't. We're only importing it to check that the library is installed with theasync_rest extra.

Now, the actual problem is that if the libraries within the try block aren't installed, we silently skip the tests for async. Happy to discuss that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So we try importingaiohttp andtransport because (1) we'll need them elsewhere for async REST, and (2) we want to set the flagGOOGLE_AUTH_AIO_INSTALLED. We only look at the flag in this file to do the right thing.

I agree with doing the right thing. What makes me uneasy is the mysterious coupling with other modules "elsewhere" in (1). I think it would be more elegant to have a package-wide function or constant that takes care of querying the right things for async REST, probably by delegating to the files that use these imports.

However, this is a test file, so no need to block on this. But we should follow up.

ohmayr reacted with thumbs up emoji
self,
request: operations_pb2.ListOperationsRequest,
*,
timeout: Optional[float] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why don't we haveretry as in the sync case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

retry isn't passed down to the auth layer i.e. it's unused. We should file an issue to fix that for the case of sync and add this as a follow up feature to the async transport class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could you file an issue to do those things?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Filed issues for each.

from google.auth.aio import credentials as ga_credentials_async

try:
import aiohttp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So we try importingaiohttp andtransport because (1) we'll need them elsewhere for async REST, and (2) we want to set the flagGOOGLE_AUTH_AIO_INSTALLED. We only look at the flag in this file to do the right thing.

I agree with doing the right thing. What makes me uneasy is the mysterious coupling with other modules "elsewhere" in (1). I think it would be more elegant to have a package-wide function or constant that takes care of querying the right things for async REST, probably by delegating to the files that use these imports.

However, this is a test file, so no need to block on this. But we should follow up.

ohmayr reacted with thumbs up emoji
self,
request: operations_pb2.ListOperationsRequest,
*,
timeout: Optional[float] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could you file an issue to do those things?

DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo(
gapic_version=BASE_DEFAULT_CLIENT_INFO.gapic_version,
grpc_version=None,
rest_version=auth_version,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I also just filed#719 to rationalize this further, separately.

ohmayr reacted with thumbs up emoji
@ohmayrohmayr merged commit3c7e43e intoasync-rest-lro-support-in-coreOct 3, 2024
5 checks passed
@ohmayrohmayr deleted the operations-rest-async-transport branchOctober 3, 2024 05:06
ohmayr added a commit that referenced this pull requestOct 7, 2024
* feat: implement `OperationsRestAsyncTransport` to support long running operations (#700)* feat: Add OperationsRestAsyncTransport to support long running operations* update TODO comment* update TODO comment* address feedback* address feedback* 🦉 Updates from OwlBot post-processorSeehttps://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md* fix mypy and lint issues* minor fix* add no cover* fix no cover tag* link coverage issue* silence coverage issue* fix statement name error* address PR feedback* address PR feedback* address PR comments---------Co-authored-by: ohmayr <omairnaveed@ymail.com>Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>* feat: implement async client for LROs (#707)* feat: implement `AbstractOperationsAsyncClient` to support long running operations* remove coverage guards* address presubmit failures* fix coverage for cancel operation* tests cleanup* fix incorrect tests* file bugs* add auth import* address PR comments* address PR comments* fix unit tests and address more comments* disable retry parameter* add retry parameter* address PR comments---------Co-authored-by: ohmayr <omairnaveed@ymail.com>Co-authored-by: ohmayr <omairn@google.com>* 🦉 Updates from OwlBot post-processorSeehttps://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md---------Co-authored-by: Anthonios Partheniou <partheniou@google.com>Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ohmayrohmayrohmayr left review comments

@vchudnov-gvchudnov-gvchudnov-g approved these changes

Assignees

@ohmayrohmayr

Labels
size: lPull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@parthea@vchudnov-g@ohmayr
[8]ページ先頭
©2009-2025 Movatter.jp