-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: Add BulkWriter buffer limit #1606
base: main
Are you sure you want to change the base?
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
@@ -167,24 +168,22 @@ enum OperationType { | |||
private final RateLimiter rateLimiter; | |||
|
|||
/** | |||
* The number of pending operations enqueued on this BulkWriter instance. An operation is | |||
* The number of in-flight operations enqueued on this BulkWriter instance. An operation is | |||
* considered pending if BulkWriter has sent it via RPC and is awaiting the result. |
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.
considered pending if BulkWriter
should this be changed to "in-flight" as well?
@Nullable | ||
public abstract Integer getMaxPendingOps(); | ||
|
||
abstract int getMaxInFlightOps(); |
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.
needs a comment here.
BTW, maybe expalin the relationship between pendingOps and InFlightOps
*/ | ||
private static final int DEFAULT_MAXIMUM_PENDING_OPERATIONS_COUNT = 500; | ||
static final int DEFAULT_MAXIMUM_IN_FLIGHT_OPERATIONS = 500; |
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.
why define it here, while it is only used in the "BulkWriterOptions" file
if (pendingOpsCount < maxPendingOpCount) { | ||
pendingOpsCount++; | ||
// Schedule the operation if the BulkWriter has fewer than the maximum number of allowed | ||
// pending operations, or add the operation to the buffer. |
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.
"allowed in-flight operations"
// operation now. This overcomes the small chance that an in-flight operation completes | ||
// before another operation has been added to buffer. | ||
if (incrementInFlightCountIfLessThanMax()) { | ||
if (!processNextBufferedOperation()) { |
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.
looks strange to do the same condition check twice to see if any operation has been completed during the last condition check.
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.
All looks good. Would love to see this change has resolved the 1 million docs OOM issue as a manual testing.
No description provided.