-
Notifications
You must be signed in to change notification settings - Fork 1.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
Initial Commit for Advanced Batching #11544
Conversation
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.
This looks great! I replied to your comment about how we can achieve hiding the constructor, which I think will be an easy change.
sdk/search/search-documents/src/searchIndexingBufferedSender.ts
Outdated
Show resolved
Hide resolved
import { createSpan } from "./tracing"; | ||
import { CanonicalCode } from "@opentelemetry/api"; | ||
|
||
const DEFAULT_BATCH_SIZE: number = 1000; |
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.
For this release, we are not allowing the user to modify these values. For next release, we will decide on what is the actual logic to be used. (Batch size vs Threshold Size, Simple Retry Count vs Retry Policy )
* @param options Options to modify batch size, auto flush and flush window. | ||
* | ||
*/ | ||
constructor(client: SearchClient<T>, options: SearchIndexingBufferedSenderOptions = {}) { |
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.
For this release, I am keeping the constructor just like Python
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.
Thanks for adding the samples! Left some updated feedback.
...search-documents/samples/typescript/src/bufferedSender/uploadDocuments/autoFlushSizeBased.ts
Show resolved
Hide resolved
...earch-documents/samples/typescript/src/bufferedSender/uploadDocuments/autoFlushTimerBased.ts
Outdated
Show resolved
Hide resolved
sdk/search/search-documents/src/searchIndexingBufferedSender.ts
Outdated
Show resolved
Hide resolved
sdk/search/search-documents/src/searchIndexingBufferedSender.ts
Outdated
Show resolved
Hide resolved
sdk/search/search-documents/src/searchIndexingBufferedSender.ts
Outdated
Show resolved
Hide resolved
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.
Great work on this! I like the design here and I am looking forward to getting feedback from customers.
@xirzec
This PR is to add fancy batch supporting for the search-documents SDK.