-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix DB to prefer batches with preferred batch size for default max_queue_d… #3298
Conversation
@tanmayv25 oops, didn't mean to approve yet. I had one requested change... |
@@ -467,7 +464,7 @@ DynamicBatchScheduler::GetDynamicBatch() | |||
return 0; | |||
} | |||
|
|||
if (delay_is_exceeded) { | |||
if (delay_is_exceeded || (pending_batch_delay_ns_ == 0)) { |
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.
if pending_batch_delay_ns_ == 0
, should the batcher NOT return 0 to wait for preferred batch or max batch?
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.
But the question then becomes how long should it wait for forming the preferred batch size. These changes only let batcher prefer a preferred batch size if one can be formed. Otherwise it will issue the request with pending batches.
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.
When delay is 0, the batcher should form the largest possible preferred batch, or if can't form any preferred batch then just the largest possible batch, and execute it immediately.
Tanmay, we should have a test with no delay where there are multiple preferred sizes and make sure that the largest preferred size is used, or when no preferred can be formed that the largest possible batch is formed. We already have these kind of tests but I guess they all use delay.
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.
I see.
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.
I am adding the tests...
2f0c9e8
to
ec3121c
Compare
…elay