Moving new Batcher API to existing Batching package#771
Moving new Batcher API to existing Batching package#771igorbernstein2 merged 3 commits intogoogleapis:masterfrom
Conversation
- Renamed to `v2.RequestBuilder` to `BatchingRequestBuilder`. - Adapted to existing BatchingSettings. - In case ElementCountThreshold or RequestByteCountThreshold is null, then using 0 as threshold level. - Defaulting to 1ms in case delayThreshold value is unset.
googlebot
added
the
cla: yes
- Cancelling the auto flush in case delaythreshold is unset by user.
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
============================================
+ Coverage 78.15% 78.15% +<.01%
+ Complexity 1101 1099 -2
============================================
Files 198 197 -1
Lines 4833 4825 -8
Branches 383 381 -2
============================================
- Hits 3777 3771 -6
Misses 886 886
+ Partials 170 168 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
============================================
+ Coverage 78.15% 78.19% +0.04%
+ Complexity 1101 1100 -1
============================================
Files 198 197 -1
Lines 4833 4825 -8
Branches 383 380 -3
============================================
- Hits 3777 3773 -4
Misses 886 886
+ Partials 170 166 -4
Continue to review full report at Codecov.
|
|
@vam-google Please have a look and let us know your thoughts. |
igorbernstein2
left a comment
igorbernstein2
left a comment
There was a problem hiding this comment.
LGTM, @vam-google please take a look
| .setElementCountThreshold(0) | ||
| .setRequestByteThreshold(0) | ||
| .setDelayThreshold(Duration.ofMillis(1)) | ||
| .setElementCountThreshold(null) |
There was a problem hiding this comment.
For my understanding, what is the difference between 0 and null? What is the behavior if you don't call any of these setters?
There was a problem hiding this comment.
Behaviorwise there is no difference, Both of these values are to disable the thresholds.
Earlier we were using v2.BatchingSettings which had primitive long that's why we used 0 for disabling, Now that we are utilizing existing BatchingSettings which allows null or greater than 0 values only So we kept the disabling with null value.
If we don't call any setter than the default values provided in the BatchingSettings will be used which is 1 element, 1 byte or 1 millisecond. Although each google-cloud-client will have their own default values.
vam-google
left a comment
vam-google
left a comment
There was a problem hiding this comment.
LGTM but with one question (can be either important or no, depending on the answer =).
| .setElementCountThreshold(1000L) | ||
| .setRequestByteThreshold(1000L) | ||
| .setElementCountThreshold(1000) | ||
| .setDelayThreshold(Duration.ofSeconds(1)) |
There was a problem hiding this comment.
Mybe it is a wrong place to looks at it, but PR description says:
"Defaulting to 1ms in case delayThreshold value is unset."
Here is seems like it defaults to 1 sec. Is something wrong here or am I just looking in a wrong place?
There was a problem hiding this comment.
I think rahul forgot to update the description of the PR :)
There was a problem hiding this comment.
I am really sorry, Have updated the description now.
We are not scheduling any auto flush if the delayThreshold is unset.
5 participants
To provide backward compatibility to stable clients( eg.
google-cloud-PubSub), we are moving the new Batcher API into the existing batching package. NowBatcherImplwill also adapt to existing configuration classesBatchingSettings&FlowControlSettings.What this change contains
v2.RequestBuildertoBatchingRequestBuilder.BatcherImplto existingBatchingSettings.elementCountThresholdorrequestByteCountThresholdis null, then using 0 as threshold level.delayThresholdvalue is unset, we are not setting any scheduler to auto flush the accumulated elements.cc: @igorbernstein2