-
Notifications
You must be signed in to change notification settings - Fork 2k
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
chore(discoveryengine): update search sample to disable auto paginate #3500
Conversation
🤖 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 use -- conventional-commit-lint bot |
General Comment - I don't think it makes sense to change the default setting for this to not paginate, because there could be a large number of search results, causing the response to be large/time consuming. Maybe add a comment for this, or make it a parameter to the sample function to show how to change this. |
To clarify, we are still doing the pagination (pageSize is still being used). This is a special behavior for NodeJS that by default it's turning the API call result into an iterable of continuous SearchResults by default (while for Python etc. it's only returning SearchResponse with a single page of SearchResult). This may cause severe issues for our case, e.g. if users set page size as 10, and there are 100 results, NodeJS library by default will try to iterate through all 100 results by sending 10 search requests in sequence (offset=0, offset=10, offset=20...) until it exhausts the full list. If you have 1K, 10K search results this could take forever and probably not what customers are expected (pageSize is not effectively respected). IMO we should always turn off the autoPaginate feature for the search use case, so we can get the single SearchResponse just for the requested page, in the above example it'll only return 10 documents. And customer can control when and what next page to fetch next. Hope this makes sense. |
Description
By default NodeJS has autoPaginate turned on, this will result in us iterate through all pages of search results, instead of just a single page restricted by pageSize.
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test
(see Testing)npm run lint
(see Style)GoogleCloudPlatform/nodejs-docs-samples
. Not a fork.