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

Rename a few properties in SearchIndexingBufferedSenderOptions #18285

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,14 @@ public SearchIndexingBufferedSenderOptions() { }
public System.Threading.CancellationToken FlushCancellationToken { get { throw null; } set { } }
public int? InitialBatchActionCount { get { throw null; } set { } }
public System.Func<T, string> KeyFieldAccessor { get { throw null; } set { } }
public int MaxRetries { get { throw null; } set { } }
public System.TimeSpan MaxRetryDelay { get { throw null; } set { } }
public System.TimeSpan RetryDelay { get { throw null; } set { } }
public int MaxRetriesPerIndexAction { get { throw null; } set { } }
public System.TimeSpan MaxThrottlingDelay { get { throw null; } set { } }
public System.TimeSpan ThrottlingDelay { get { throw null; } set { } }
}
public partial class SearchIndexingBufferedSender<T> : System.IAsyncDisposable, System.IDisposable
{
protected SearchIndexingBufferedSender() { }
public SearchIndexingBufferedSender(Azure.Search.Documents.SearchClient searchClient, Azure.Search.Documents.SearchIndexingBufferedSenderOptions<T> options = null) { }
Copy link
Member

Choose a reason for hiding this comment

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

We didn't also remove the helper method on SearchClient?

public virtual System.Uri Endpoint { get { throw null; } }
public virtual string IndexName { get { throw null; } }
public virtual string ServiceName { get { throw null; } }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ again.

```C# Snippet:Azure_Search_Documents_Tests_Samples_Sample05_IndexingDocuments_BufferedSender1
await using SearchIndexingBufferedSender<Product> indexer =
searchClient.CreateIndexingBufferedSender<Product>();
new SearchIndexingBufferedSender<Product>(searchClient);
await indexer.UploadDocumentsAsync(GenerateCatalog(count: 100000));
```

Expand Down
4 changes: 2 additions & 2 deletions sdk/search/Azure.Search.Documents/src/Batching/Publisher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,10 @@ public int IndexingActionsCount
protected CancellationToken PublisherCancellationToken { get; }

/// <summary>
/// Gets a value indicating the number of actions to group into a batch
/// Gets or sets a value indicating the number of actions to group into a batch
/// when tuning the behavior of the publisher.
/// </summary>
protected int BatchActionCount { get; } // TODO: Not automatically tuning yet
protected int BatchActionCount { get; set; }

/// <summary>
/// Gets a value indicating the number of bytes to use when tuning the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class SearchIndexingBufferedSender<T> : IDisposable, IAsyncDisposable
/// The single publisher responsible for submitting requests.
/// </summary>
#pragma warning disable CA2213 // Member should be disposed. Disposed in DisposeAsync
private SearchIndexingPublisher<T> _publisher;
private readonly SearchIndexingPublisher<T> _publisher;
#pragma warning restore CA2213 // Member should be disposed. Disposed in DisposeAsync

/// <summary>
Expand Down Expand Up @@ -108,15 +108,18 @@ public class SearchIndexingBufferedSender<T> : IDisposable, IAsyncDisposable
protected SearchIndexingBufferedSender() { }

/// <summary>
/// Creates a new instance of the SearchIndexingBufferedSender.
/// Creates a new instance of <see cref="SearchIndexingBufferedSender{T}"/> that
/// can be used to index search documents with intelligent batching,
/// automatic flushing, and retries for failed indexing actions.
/// </summary>
/// <param name="searchClient">
/// The SearchClient used to send requests to the service.
/// </param>
/// <param name="options">
/// Provides the configuration options for the sender.
/// The <see cref="SearchIndexingBufferedSenderOptions{T}"/> to
/// customize the sender's behavior.
Mohit-Chakraborty marked this conversation as resolved.
Show resolved Hide resolved
/// </param>
internal SearchIndexingBufferedSender(
public SearchIndexingBufferedSender(
Copy link
Member

Choose a reason for hiding this comment

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

Should this still be public if you're making it creatable from the SearchClient?

Copy link
Member

Choose a reason for hiding this comment

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

...and if you're not calling the helper from tests anymore (at least, I don't see it here but maybe GitHub is hiding it), I'm puzzled as to why tests are now passing.

Copy link
Contributor Author

@Mohit-Chakraborty Mohit-Chakraborty Feb 6, 2021

Choose a reason for hiding this comment

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

Making it public is a part of the requested change. This is being used from the samples now.
The other part of removing the helper will be tackled in a different PR.

The helper is used heavily in the tests. That is the reason for the now small diff in BatchingTests.cs.

SearchClient searchClient,
SearchIndexingBufferedSenderOptions<T> options = null)
{
Expand All @@ -131,9 +134,9 @@ internal SearchIndexingBufferedSender(
options.AutoFlushInterval,
options.InitialBatchActionCount,
options.BatchPayloadSize,
options.MaxRetries,
options.RetryDelay,
options.MaxRetryDelay,
options.MaxRetriesPerIndexAction,
options.ThrottlingDelay,
options.MaxThrottlingDelay,
options.FlushCancellationToken);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public class SearchIndexingBufferedSenderOptions<T>
/// to control the number of attempts we will make to submit an indexing
/// action.
/// </summary>
public int MaxRetries { get; set; } = 3;
public int MaxRetriesPerIndexAction { get; set; } = 3;

/// <summary>
/// The initial retry delay. The delay will increase exponentially with
Expand All @@ -73,7 +73,7 @@ public class SearchIndexingBufferedSenderOptions<T>
/// is used to add delay between additional batch submissions when our
/// requests are being throttled by the service.
/// </summary>
public TimeSpan RetryDelay { get; set; } = TimeSpan.FromSeconds(0.8);
public TimeSpan ThrottlingDelay { get; set; } = TimeSpan.FromSeconds(0.8);

/// <summary>
/// The maximum permissible delay between retry attempts. Note that
Expand All @@ -82,7 +82,7 @@ public class SearchIndexingBufferedSenderOptions<T>
/// property is used to add delay between additional batch
/// submissions when our requests are being throttled by the service.
/// </summary>
public TimeSpan MaxRetryDelay { get; set; } = TimeSpan.FromMinutes(1);
public TimeSpan MaxThrottlingDelay { get; set; } = TimeSpan.FromMinutes(1);

/// <summary>
/// Gets or sets a function that can be used to access the index key
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,11 @@ protected override async Task<bool> OnSubmitBatchAsync(IList<PublisherAction<Ind
catch (RequestFailedException ex) when (ex.Status == 413) // Payload Too Large
{
// Split the batch and try with smaller payloads
int half = (int)Math.Floor((double)batch.Count / 2.0);
var smaller = new List<PublisherAction<IndexDocumentsAction<T>>>(batch.Take(half));
foreach (PublisherAction<IndexDocumentsAction<T>> action in batch.Skip(half))
// Update 'BatchActionCount' so future submissions can avoid this error.
BatchActionCount = (int)Math.Floor((double)batch.Count / 2.0);

var smaller = new List<PublisherAction<IndexDocumentsAction<T>>>(batch.Take(BatchActionCount));
foreach (PublisherAction<IndexDocumentsAction<T>> action in batch.Skip(BatchActionCount))
{
// Add the second half to the retry queue without
// counting this as a retry attempt
Expand Down
6 changes: 3 additions & 3 deletions sdk/search/Azure.Search.Documents/src/SearchClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public class SearchClient
/// Gets the <see cref="Azure.Core.Pipeline.ClientDiagnostics"/> used
/// to provide tracing support for the client library.
/// </summary>
private ClientDiagnostics ClientDiagnostics { get; }
internal ClientDiagnostics ClientDiagnostics { get; }

/// <summary>
/// Gets the REST API version of the Search Service to use when making
Expand Down Expand Up @@ -729,7 +729,7 @@ public virtual Response<SearchResults<T>> Search<T>(
/// the <see cref="AsyncPageable{T}.AsPages(string, int?)"/> method.
/// </para>
/// </remarks>
public async virtual Task<Response<SearchResults<T>>> SearchAsync<T>(
public virtual async Task<Response<SearchResults<T>>> SearchAsync<T>(
string searchText,
SearchOptions options = null,
CancellationToken cancellationToken = default) =>
Expand Down Expand Up @@ -1194,7 +1194,7 @@ public virtual Response<IndexDocumentsResult> IndexDocuments<T>(
/// <see cref="AggregateException"/> that's thrown on partial failure.
/// </para>
/// </remarks>>
public async virtual Task<Response<IndexDocumentsResult>> IndexDocumentsAsync<T>(
public virtual async Task<Response<IndexDocumentsResult>> IndexDocumentsAsync<T>(
IndexDocumentsBatch<T> batch,
IndexDocumentsOptions options = null,
CancellationToken cancellationToken = default) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1087,8 +1087,8 @@ public async Task Behavior_MaxRetries()
client.CreateIndexingBufferedSender(
new SearchIndexingBufferedSenderOptions<SimpleDocument>()
{
MaxRetries = 5,
MaxRetryDelay = TimeSpan.FromSeconds(1)
MaxRetriesPerIndexAction = 5,
MaxThrottlingDelay = TimeSpan.FromSeconds(1)
});

// Keep 503ing to count the retries
Expand Down Expand Up @@ -1116,8 +1116,8 @@ public async Task Behavior_RetryDelay()
client.CreateIndexingBufferedSender(
new SearchIndexingBufferedSenderOptions<SimpleDocument>()
{
MaxRetries = 1,
RetryDelay = TimeSpan.FromSeconds(3)
MaxRetriesPerIndexAction = 1,
ThrottlingDelay = TimeSpan.FromSeconds(3)
});

// Keep 503ing to trigger delays
Expand Down Expand Up @@ -1148,8 +1148,8 @@ public async Task Behavior_MaxRetryDelay()
client.CreateIndexingBufferedSender(
new SearchIndexingBufferedSenderOptions<SimpleDocument>()
{
MaxRetries = 10,
MaxRetryDelay = TimeSpan.FromSeconds(1)
MaxRetriesPerIndexAction = 10,
MaxThrottlingDelay = TimeSpan.FromSeconds(1)
});

// Keep 503ing to trigger delays
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public async Task BufferedSender()
{
#region Snippet:Azure_Search_Documents_Tests_Samples_Sample05_IndexingDocuments_BufferedSender1
await using SearchIndexingBufferedSender<Product> indexer =
searchClient.CreateIndexingBufferedSender<Product>();
new SearchIndexingBufferedSender<Product>(searchClient);
await indexer.UploadDocumentsAsync(GenerateCatalog(count: 100000));
#endregion
}
Expand Down