Skip to content

Design a pageable solution for list operations #341

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
heaths opened this issue Aug 5, 2021 · 7 comments
Closed

Design a pageable solution for list operations #341

heaths opened this issue Aug 5, 2021 · 7 comments
Labels
Azure.Core The azure_core crate design-discussion An area of design currently under discussion and open to team and community feedback. Docs

Comments

@heaths
Copy link
Member

heaths commented Aug 5, 2021

Like in our other languages' SDKs, we need a solution like Pageable<T> that can fetch more pages as needed. It should do so automatically while providing callers the ability to fetch page by page if they wish to.

Ideally needed for #218

@MindFlavor
Copy link
Contributor

MindFlavor commented Aug 11, 2021

I think the solution we are currently using is good: we simply return a Stream.
Streams correctly inform the caller of the availability or more data and allow to keep receiving data in a very clean way. We use it both in the old and new architecture. For example (pre pipeline):

pub fn stream(
self,
) -> impl Stream<Item = Result<ListBlobsResponse, Box<dyn std::error::Error + Sync + Send>>> + 'a
{
#[derive(Debug, Clone, PartialEq)]
enum States {
Init,
NextMarker(NextMarker),
}
unfold(Some(States::Init), move |next_marker: Option<States>| {
let req = self.clone();
async move {
debug!("next_marker == {:?}", &next_marker);
let response = match next_marker {
Some(States::Init) => req.execute().await,
Some(States::NextMarker(next_marker)) => {
req.next_marker(next_marker).execute().await
}
None => return None,
};
// the ? operator does not work in async move (yet?)
// so we have to resort to this boilerplate
let response = match response {
Ok(response) => response,
Err(err) => return Some((Err(err), None)),
};
let next_marker = response.next_marker.clone().map(States::NextMarker);
Some((Ok(response), next_marker))
}
})
}

We used to expose both the streaming and non streaming functions (as seen above) but we are discussing with @yoshuawuyts in #340 if we should only offer the streaming function from now on.
The only possible disadvantage of giving back a stream is the implicit lifetime attached to it that might make the struct difficult to work with unless immediately consumed.

@yoshuawuyts, @rylev should consider this more? I didn't think about it before but a streaming function can be difficult to work with due to lifetimes.

@MindFlavor MindFlavor added design-discussion An area of design currently under discussion and open to team and community feedback. guidelines labels Aug 11, 2021
@heaths
Copy link
Member Author

heaths commented Aug 13, 2021

How easily can that be extracted? In so our other SDKs, implementations really only need to provide a way to get the next link or page token, and there are helpers because there's a couple very common ways...by design: services are supposed to do it one way for new APIs anyway.

What you said there seems very involved. I assume want every implementation to have to write that code. Harder to maintain, not to mention harder to improve centrally if needs be.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Aug 26, 2021

It seems like this thread touches on several topics which may be useful to tease apart:

  1. What should our external API look like?
  2. Can we reuse code between implementations, and possibly codegen too?

External API

Right now list_databases is returning impl Stream<Item = DatabaseResponse>. This seems eerily close to the Pageable abstraction C# provides. But using impl Stream instead of a named type carries the inherent disadvantage that it cannot be, well, named. Which means users building libraries on top of azure will be stuck Boxing the streams, which is not great. It seems to me that ideally we would indeed expose a named stream type here.

code reuse between implementations

I certainly hope we can reuse code between implementations, but having worked on #340 it might prove to be non-trivial. In yesterday's meeting we touched on this topic, and it seems like there's definitely a desire to. Regardless whether we expose impl Stream or StreamType<T>, it seems folks generally agree it makes sense to reuse code for this.

Conclusion

I'd like to propose that once we merge #346 and #340, we look at creating a shared abstraction. Ideally we could expose some equivalent to Pageable that implements Stream (for the reasons outlined above). But if that turns out to be too difficult, being able to share the implementation's internals would be a big leap forward already.

Does that sounds reasonable?

@cataggar cataggar self-assigned this Oct 5, 2021
@cataggar cataggar removed their assignment Oct 22, 2021
@cataggar cataggar added this to the services 0.2.0 milestone Oct 22, 2021
@cataggar cataggar added the Azure.Core The azure_core crate label Nov 8, 2021
@cataggar
Copy link
Member

cataggar commented Nov 8, 2021

Ideally we could expose some equivalent to Pageable that implements Stream (for the reasons outlined above)
❤️

All of the generated services that have operation responses with x-ms-pageable #218 would benefit from a shared abstraction. Here are what some other Azure SDKs do:

Azure SDK for CPP has a Azure::PagedResponse<T> that was added in May:
https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/core/azure-core/CHANGELOG.md

Azure SDK for JavaScript has:

import { PagedAsyncIterableIterator } from "@azure/core-paging";

and a new experimental Azure Rest Core Paging library.

Azure SDK for Go has done it a couple of ways.

@heaths
Copy link
Member Author

heaths commented Nov 15, 2021

We might want some heuristic scanning as well. I can't say for sure for this Microsoft swagger extensions, but for some other extensions some services aren't always so consistent. That said, if other languages' generators use only this I see no reason to be different and try to support more.

@cataggar
Copy link
Member

cataggar commented Jan 1, 2022

Basic guidelines here:
https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#98-pagination

azure_core::Pageable was added in #534.

@cataggar
Copy link
Member

cataggar commented May 2, 2022

Closing, since azure_core::Pageable exists now.

@cataggar cataggar closed this as completed May 2, 2022
@heaths heaths added the Docs label Jan 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core The azure_core crate design-discussion An area of design currently under discussion and open to team and community feedback. Docs
Projects
None yet
Development

No branches or pull requests

4 participants