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.

Sign up for GitHub

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

Ensure only a single action for a document is added to a batch #18469

Merged
merged 3 commits into from
Feb 6, 2021
Merged

Ensure only a single action for a document is added to a batch #18469

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8bf25b
Ensure only a single action for a document is added to a batch
Mohit-Chakraborty Feb 5, 2021
48db0c8
Add test to validate batch splitting behavior
Mohit-Chakraborty Feb 5, 2021
faed37e
Add a test to verify case-sensitive nature of keys
Mohit-Chakraborty Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions sdk/search/Azure.Search.Documents/src/Batching/Publisher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,14 @@ await SubmitBatchAsync(
// Returns whether the batch is now full.
bool FillBatchFromQueue(List<PublisherAction<T>> batch, Queue< PublisherAction<T>> queue)
{
// TODO: Consider tracking the keys in the batch and requiring
// them to be unique to avoid error alignment problems
HashSet<string> documentIdentifiers = new HashSet<string>(StringComparer.Ordinal);

while (queue.Count > 0)
{
if (batch.Count < BatchActionCount)
// Stop filling the batch if we run into an action for a document that is already in the batch.
// We want to keep the actions ordered and map any errors accurately to the documents,
// so we need to split the batch on encountering an action for a previously queued document.
if ((batch.Count < BatchActionCount) && documentIdentifiers.Add(queue.Peek().Key))
Copy link
Member

Choose a reason for hiding this comment

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

Imagine you've got a sequence of pending actions like <(Upload A), (Upload B), (Merge A), (Upload C), (Merge B)>.

The code as implemented will create a batch of <(Upload A), (Upload B)> and leave <(Merge A), (Upload C), (Merge B)> pending for the next batch.

I was hoping we could instead create a batch of <(Upload A), (Upload B), (Upload C)> and leave <(Merge A), (Merge B)> pending for the next batch.

It would be a little more complicated because .NET's Queue isn't amenable to cutting in line. It won't be a breaking change to do this in the future though, so let's ship what you have here and consider fixing this as an optimization next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I was under the (wrong) impression that the order cannot be mucked with. But it makes sense that only the order of operations for the same document need be respected.
Can there ever be a case that relies on the ordering of the whole set of operations? Like, (a completely made up example) - there's a hook on the service side that executes additional operations and relies on the relationship between the documents? So, (Upload C) becomes an issue if it happens before (Merge A)?

{
batch.Add(queue.Dequeue());
}
Expand Down