Skip to content
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

Search: ListIndexes operation should return a pageable collection #11053

Closed
heaths opened this issue Apr 4, 2020 · 6 comments · Fixed by #11517
Closed

Search: ListIndexes operation should return a pageable collection #11053

heaths opened this issue Apr 4, 2020 · 6 comments · Fixed by #11517
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Search
Milestone

Comments

@heaths
Copy link
Member

heaths commented Apr 4, 2020

Should we make all these Get*s Pageable<T>? We can discuss it after this Preview with Bruce to get an idea of how these features might grow over time.

Originally posted by @tg-msft in #11049

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Apr 4, 2020
@heaths
Copy link
Member Author

heaths commented Apr 4, 2020

Currently, the model returns themselves aren't pageable, but if they ever became pageable we'd be ready without a breaking change. That said, I imagine that all of these in the SearchServiceClient wouldn't yield enough results to bother making them pageable.

@heaths heaths added Client This issue points to a problem in the data-plane of the library. Search labels Apr 4, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Apr 4, 2020
@heaths heaths added this to the [2020] May milestone Apr 4, 2020
@heaths
Copy link
Member Author

heaths commented Apr 21, 2020

@brjohnstmsft might some of these methods ever become pageable, such that we should make the return types pageable now so these aren't breaking changes later (well, we wouldn't break the API, but would have to add overloads which isn't ideal).

@brjohnstmsft
Copy link
Member

brjohnstmsft commented Apr 22, 2020

@heaths Probably not. The limits for these resources are documented here, and the numbers aren't high enough to justify paging.

@heaths
Copy link
Member Author

heaths commented Apr 22, 2020

@brjohnstmsft Index and Indexer limits for S3 were pretty high. If there's any chance the service would return a nextLink or something in the future, we should make it pageable now.

/cc @pakrym

@brjohnstmsft
Copy link
Member

@heaths The max indexer count is 200; that's not high. Indexes can get up to 3000 in case of S3HD. So far that hasn't been enough to motivate us to introduce paging. We're looking at allowing more fields per index, which would increase the response size for this API, so maybe we can consider nextLink in the future. If you want to be maximally defensive against breaking changes, maybe make listing indexes pageable.

For the other resources, I would say whether you make it pageable or not should depend on how much that degrades the developer experience, because the chances of us making those APIs return nextLink is very low.

@heaths
Copy link
Member Author

heaths commented Apr 22, 2020

No, 200 isn't that high but if properties increase that still gets pretty big. Key Vault, in comparison, has pretty small pages. Making only GetIndexes pageable carries the concern about inconsistency, since enumerating a pageable method uses different code.

@heaths heaths self-assigned this Apr 22, 2020
@heaths heaths changed the title Consider making list operations in SearchServiceClient pageable Search: ListIndexes operation should return a pageable collection Apr 22, 2020
heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Apr 22, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Search
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants