Skip to content

Conversation

@jviau
Copy link
Member

@jviau jviau commented Aug 29, 2022

This PR introduces support for querying multiple orchestration instances over gRPC. The implementation also takes a page out of Azure SDKs book by copying their AsyncPageable design (pun intended). This abstraction layer is important as we want to accomplish 2 primary goals with our async enumerable implementation:

  1. I want to enumerate all pages right away: AsyncPageable<T> implements IAsyncEnumerable<T>.
  2. I want to enumerate a page at a time and/or pause/resume paging: AsyncPageable<T>.AsPages() allows for this via .AsPages(continuationToken).FirstAsync() from System.Linq.Async package.
    • This means queries can be paused/resumed/rehydrated in an abstract fashion. This is important for DTFx usage itself, with this design someone can easily break a query down into a page per activity (so we don't have to serialize it all at once!).
    • We can even provide an example of an orchestration yield results in pages via AsyncPageable<T> itself.

resolves #2

@jviau jviau requested a review from cgillum September 6, 2022 20:45
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Looking good so far! I'll admit that this pageable stuff is new to me. Will you be able to add some tests to help verify the functionality (and provide examples of how to use the API)?

@jviau
Copy link
Member Author

jviau commented Sep 8, 2022

The latest iteration includes a bunch of changes to other files as part of adding unit tests.

  • Added FluentAssertions and System.Linq.Async package to test projects
  • Added / rearranged build/eng files so that unit test projects are strong name signed and can have InternalsVisibleTo added.
    • Added some msbuild settings to test projects, like implicit usings. There are using changes to other files as a result.
  • Added unit tests and an integration test.

@jviau
Copy link
Member Author

jviau commented Sep 8, 2022

@cgillum the integration test I added shows an example of the benefit of this abstraction layer. Instead of the static switch case implementation there though, an activity would in turn call an API or something that itself is paged. For a CosmosDB query, they could return a single page at a time there. For a query to the API I just introduced, the implementation would be something like return client.GetInstances({ continuation, pageSize }).AsPages().FirstAsync();.

This won't really be exposed right away to customers as the Pageable helper is internal still. I want to think about the ConfigureAwait issue some more before exposing that. Ideally we would also introduce a strongly typed request-style API for the OrchestrationContext, and then we can add overloads for activities which return Page<T> and automatically wrap them into an orchestration-friendly AsyncPageable<T> for the caller.

@jviau jviau requested a review from cgillum September 14, 2022 21:09
Copy link
Member

@cgillum cgillum 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 adding the new tests! Left a few comments, but nothing blocking.

@jviau jviau merged commit 6e693a6 into microsoft:main Sep 19, 2022
@jviau jviau deleted the multi-instance-query branch September 19, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-instance query support

2 participants