Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Search batching using bfetch #83418
[Search] Search batching using bfetch #83418
Changes from 25 commits
20bb351
8b4eb1c
9eebded
812a06b
aaa3122
b1a2cf2
2cdfc47
e8fc98f
993e385
e89a46f
b727e49
3de838c
17de9fa
56e7a46
5aaeefa
c393416
a1b2979
811da0a
162473d
8c83657
662b810
41f6406
cbd8106
1ec890a
3e334a7
c7aebdf
8abff48
b4193e2
9d16439
4a1e69c
bcd894f
d57b8dc
6b9a4f8
4bd2a96
e9103b7
3fcac6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand it correctly that:
signal
, which can abort that specific item?If that is the case, I think it is better to implement it as it was before with the counters. Putting back the
responsesReceived
counter and possibly introducing a counter for aborted items, likenumberOfAbortedItems
. And then use those counts to abort the whole batch when needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You understood correctly and I thought so too at first, but the counters are problematic, because the same request can be received and then aborted, you'd need to be very careful not to count the same request twice towards both counters.
I think it's simpler and more straight forward with the implementation I did here.
Do you see any problems with my implementation, other than stlistically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small problem, which practically might not be an issue. The
abortController.abort()
is called whenever all items in the batch have somehow completed—either resolved or rejected, or aborted; in any combination.But my understanding is that we don't need to call
abortController.abort()
when, for example, all items successfully resolved. We need to callabortController.abort()
only when all items have completed and at least one of them completed due to an abort signal.Do you think we should be that specific? Or aborting the request every time, even when it already finished successfully is a noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it matters, as
fromStreamingXhr
makes sure not to apply abort to an already complete xhr request.We could track each resolution separately, but I don't think it's worth it.