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: implement async client for LROs#707

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 24, 2024
edited
Loading

Towards b/362946071

This PR depends on#700 and#701

@product-auto-labelproduct-auto-labelbot added the size: xlPull request size is extra large. labelSep 24, 2024
@partheaparthea changed the base branch frommain tooperations-rest-async-transportSeptember 24, 2024 00:24
@conventional-commit-lint-gcfConventional Commit Lint GCF
Copy link

conventional-commit-lint-gcfbot commentedSep 24, 2024
edited
Loading

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or useautomerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@partheapartheaforce-pushed theoperations-rest-async-client branch 3 times, most recently from1492985 tocb2eab0CompareSeptember 25, 2024 15:32
@partheaparthea changed the base branch fromoperations-rest-async-transport tomainSeptember 25, 2024 17:35
@partheapartheaforce-pushed theoperations-rest-async-client branch from787f0ba to35d88d7CompareSeptember 25, 2024 17:55
@partheaparthea changed the title[WIP] Operations rest async clientfeat: Add AbstractOperationsAsyncClientSep 25, 2024
@ohmayrohmayrforce-pushed theoperations-rest-async-client branch from5ea5e27 to4d75ccaCompareOctober 1, 2024 20:30
@ohmayrohmayr changed the base branch frommain tooperations-rest-async-transportOctober 1, 2024 20:31
@ohmayrohmayrforce-pushed theoperations-rest-async-client branch 2 times, most recently from5e6c365 to74e9bb8CompareOctober 1, 2024 21:02
@ohmayrohmayr changed the base branch fromoperations-rest-async-transport tomainOctober 1, 2024 21:03
@ohmayrohmayr marked this pull request as ready for reviewOctober 1, 2024 21:09
@ohmayrohmayr requested review froma team ascode ownersOctober 1, 2024 21:09
@ohmayrohmayrforce-pushed theoperations-rest-async-client branch from4f3671c to2af34aaCompareOctober 2, 2024 20:45
@ohmayrohmayr changed the base branch frommain tooperations-rest-async-transportOctober 2, 2024 21:26
Base automatically changed fromoperations-rest-async-transport toasync-rest-lro-support-in-coreOctober 3, 2024 05:06
@ohmayrohmayr changed the titlefeat: Add AbstractOperationsAsyncClientfeat: implement async client for LROsOct 3, 2024
@ohmayrohmayrforce-pushed theoperations-rest-async-client branch from5564d01 tof448498CompareOctober 3, 2024 05:19
Copy link
Contributor

@vchudnov-gvchudnov-g left a comment

Choose a reason for hiding this comment

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

I have some initial comments. I'll continue next week.

The main thing I want to work out in my head is the class hierarchy, and how it pertains to {sync, async} × {grpc, rest}.

What I've worked out so far is this class hierarchy, but I don't see where gRPC fits in. I need to think about this more, and it might be worth a discussion:

# do any of these include gRPC?AbstractOperationsBaseClient   o----  OperationsRestTransport   ^ ^                         o----  OperationsRestAsyncTransport   | |          | |          | AbstractOperationsClient       # sync   |   |   AbstractOperationsAsyncClientListOperationsPagerBase  ^  ^  |  |  |  |  |  ListOperationsPager           # sync  |  |  ListOperationsAsyncPager

super().__init__(
credentials=credentials, # type: ignore
# NOTE: If a transport is not provided, we force the client to use the async
# REST transport, as it should.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why should it? This abstract operations class does not say it's REST-specific, so why couldn't we go with gRPC async? We should see default to one of the ones that are supported (have the right dependencies); if both gRPC and REST are possible, sure, we could have a policy as to which one we choose for the default. As per my other comment, if the plan is to do this as we clean up this code, etc., let's add a TODO with link to a tracking issue that includes setting the right default.

  • Regardless, I would remove the final clause:

Suggested change
# REST transport, as it should.
# REST transport.
  • Also, update the class doc above so instead if "If set to None, a transport is chosen automatically", it says "If set to None, this defaults to 'rest_asyncio'".

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a TODO to add the right default. This is an async REST client so we're setting the right default value i.e.rest_asyncio.

What we may instead want to do is update the name / docstring of the class to make it clear that this is specific to async REST.

In future, we may want to spend some time to investigate if we can combine different transport and client class for gRPC and REST that we maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if this is an async rest client, why do we have a transport parameter at all? What do we do if thetransport is not an async rest transport (ours or user-defined)? Might be worth filing a follow-up issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Users can still configure an instance of an async transport class but I agree that it is a bit weird. Can add a note about it in the issue that i file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed:#726

try:
from google.auth.aio import credentials as ga_credentials # type: ignore
except ImportError as e: # pragma: NO COVER
raise ImportError(
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the file and the name of the class do not make it clear that this is REST-specific. So async should work if at least one of { (REST dependencies), (gRPC dependencies) } are available. So it seems to me we should check for both sets of dependencies being absent before we error.

That, at least, when we get to the steady state of having everything implemented and cleaned up. If that's the reason we're not doing that now, let's add a TODO with a link to a tracking issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. The name doesn't specify it but it is indeedREST specific. I agree that it's confusing. The name used here is consistent to what we have for the sync REST client / transport. We can have a discussion on what we want to name our async client / transport.

Updating the sync client name would be a breaking change.

page_token: Optional[str] = None,
timeout: Optional[float] = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> pagers.ListOperationsAsyncPager:
Copy link
Contributor

Choose a reason for hiding this comment

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

AbstractOperationsClient has acompression parameter. Since This abstract class is, I assume, also applicable to gRPC, we should accept that parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

AbstractOperationsClient shouldn't have acompression parameter since it's aREST only client. We have a separate client forgRPC.

@ohmayrohmayr changed the base branch fromasync-rest-lro-support-in-core tomainOctober 7, 2024 02:10
@ohmayrohmayr changed the base branch frommain toasync-rest-lro-support-in-coreOctober 7, 2024 05:10
@ohmayr
Copy link
Contributor

@vchudnov-g I've addressed all of your comments.

The only thing that's left and is causing a bit of confusion is the name of client/transport class. Let's discuss this, address it accordingly, and get this over the line since I believe it's not a big change.

Copy link
Contributor

@vchudnov-gvchudnov-g left a comment
edited
Loading

Choose a reason for hiding this comment

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

Yes, let's have a clearer naming convention for files and classes. We want to cover {sync, async} × {REST, gRPC}.

In the ideal world, we would have prefixes for each of those two dimensions:

Foo (egTransport orClient)
SyncFooAsyncFoo
gRPCSyncFooRestSyncFoogRPCAsyncFooRestAsyncFoo

Given historical naming and wanting to keep backward-compatibility, maybe we want something like this?:

BaseFoo (egBaseTransport orBaseClient)
BaseSyncFooBaseAsyncFoo
FooRestSyncFooAsyncFooRestAsyncFoo

Let's discuss the right names.....

Also, we should consider that we could have the "ideal" names in their own logical namespace, and assign the legacy names in the legacy namespace, e.e.:
transports.Foo = transports.ideal.gRPCSyncFoo
transports.AsyncFoo = transports.ideal.gRPCAsyncFoo
We'd have to figure out what name to use; it obviously wouldn't literally be "ideal".
Luckily, we can do this now or later, so we have options.

Copy link
Contributor

@vchudnov-gvchudnov-g left a comment

Choose a reason for hiding this comment

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

Thanks for the discussion on the class hierarchy now and how we want it to evolve. Googlers, see b/372066378

super().__init__(
credentials=credentials, # type: ignore
# NOTE: If a transport is not provided, we force the client to use the async
# REST transport, as it should.
Copy link
Contributor

Choose a reason for hiding this comment

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

So if this is an async rest client, why do we have a transport parameter at all? What do we do if thetransport is not an async rest transport (ours or user-defined)? Might be worth filing a follow-up issue.

@ohmayrohmayr merged commit043785e intoasync-rest-lro-support-in-coreOct 7, 2024
5 checks passed
@ohmayrohmayr deleted the operations-rest-async-client branchOctober 7, 2024 22:34
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

@vchudnov-gvchudnov-g

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

Successfully merging this pull request may close these issues.

3 participants
@parthea@ohmayr@vchudnov-g

[8]ページ先頭

©2009-2025 Movatter.jp