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

Jump to bottom

Search Fancy Batching #15750

Merged
merged 11 commits into from Oct 9, 2020
Merged

Search Fancy Batching #15750

merged 11 commits into from Oct 9, 2020

Conversation

tg-msft
Copy link
Member

@tg-msft tg-msft commented Oct 6, 2020

Adds SearchIndexingBufferedSender to index search documents with intelligent batching, automatic flushing, and retries for failed indexing actions.

Fixes #11161.

@tg-msft tg-msft requested review from heaths and pakrym October 6, 2020 21:16
@ghost ghost added the Search label Oct 6, 2020
@tg-msft
Copy link
Member Author

tg-msft commented Oct 6, 2020

Still finishing up the tests, but wanted to get the bulk of it out.

@tg-msft
Copy link
Member Author

tg-msft commented Oct 6, 2020

And to review, I'd suggest files that don't start with "Search", followed by Publisher*, followed by SearchIndexingPublisher, followed by SearchIndexingBufferedSender.

pakrym
pakrym reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@pakrym pakrym left a comment

Choose a reason for hiding this comment

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

Some initial comments about parallel primitives. Reviewing the rest.

sdk/search/Azure.Search.Documents/src/Batching/PublisherAction{T}.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/ProducerBarrierSlim.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/ManualRetryDelay.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender{T}.cs Outdated Show resolved Hide resolved
heaths
heaths approved these changes Oct 6, 2020
View reviewed changes
sdk/search/Azure.Search.Documents/api/Azure.Search.Documents.netstandard2.0.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/AsyncEventExtensions.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/AsyncEventExtensions.cs Outdated
// be surfaced
if (joined.Exception?.InnerExceptions?.Count > 1)
{
throw joined.Exception;
Copy link
Member

Choose a reason for hiding this comment

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

This will overwrite the stack. Instead, throw it as an inner exception to a new exception to preserve the original stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at this again, I remembered why I didn't care. This is the AggregateException created and thrown by awaiting a few lines up. I don't mind shifting its callstack down here because all of its InnerExceptions with the user's callstacks are fine.

sdk/search/Azure.Search.Documents/src/Batching/InitializationBarrierSlim.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/Publisher{T}.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSenderOptions{T}.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender{T}.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender{T}.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender{T}.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/src/Batching/SearchIndexingBufferedSender{T}.cs Outdated Show resolved Hide resolved
@pakrym
Copy link
Contributor

pakrym commented Oct 7, 2020

Nice and clean! Me likey.

@tg-msft
Copy link
Member Author

tg-msft commented Oct 7, 2020

/azp run net - search - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tg-msft
Search Fancy Batching
cb0c1f1 
Adds SearchIndexingBufferedSender to index search documents with intelligent
batching, automatic flushing, and retries for failed indexing actions.

Fixes Azure#11161.
@tg-msft
Applying CR feedback
23bd96b
@tg-msft
More CR feedback
22b3ded
@tg-msft
Adding a basic sample and tests
1aa9b80
@tg-msft tg-msft marked this pull request as ready for review October 9, 2020 16:41
heaths
heaths approved these changes Oct 9, 2020
View reviewed changes
sdk/search/Azure.Search.Documents/samples/Sample05_IndexingDocuments.md Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Batching/AsyncEventExtensionsTests.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Batching/AsyncEventExtensionsTests.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Batching/BatchingTests.cs Outdated Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Batching/BatchingTests.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Batching/BatchingTests.cs Show resolved Hide resolved
sdk/search/Azure.Search.Documents/tests/Samples/Sample05_IndexingDocuments.cs
#region Snippet:Azure_Search_Documents_Tests_Samples_Sample05_IndexingDocuments_GenerateCatalog
public IEnumerable<Product> GenerateCatalog(int count = 1000)
{
// Adapted from https://weblogs.asp.net/dfindley/Microsoft-Product-Name-Generator
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should put this in Azure.Core.TestFramework. 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea for a future release - a lot of the internal test frameworks I've used had really good random generators for names, addresses, etc., that made it easy to set things like this up out of the box.

sdk/search/Azure.Search.Documents/tests/Utilities/SearchTestBase.cs Show resolved Hide resolved
@tg-msft tg-msft merged commit 4a68f15 into Azure:master Oct 9, 2020
suhas92 pushed a commit to suhas92/azure-sdk-for-net that referenced this pull request Oct 12, 2020
@tg-msft @suhas92
Search Fancy Batching (Azure#15750)
a2f6cc5 
Adds SearchIndexingBufferedSender to index search documents with intelligent
batching, automatic flushing, and retries for failed indexing actions.

Fixes Azure#11161.
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
@tg-msft @annelo-msft
Search Fancy Batching (Azure#15750)
750083b 
Adds SearchIndexingBufferedSender to index search documents with intelligent
batching, automatic flushing, and retries for failed indexing actions.

Fixes Azure#11161.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heaths heaths heaths approved these changes

@pakrym pakrym pakrym left review comments

@arv100kri arv100kri Awaiting requested review from arv100kri

@bleroy bleroy Awaiting requested review from bleroy

@brjohnstmsft brjohnstmsft Awaiting requested review from brjohnstmsft

@KrzysztofCwalina KrzysztofCwalina Awaiting requested review from KrzysztofCwalina

Assignees
No one assigned
Labels
Search
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Search: Design and implement batching operation for document operations
3 participants
@tg-msft @pakrym @heaths