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

fix: BulkWriter: apply rate limiter before sending batch #1451

Merged
merged 4 commits into from Mar 16, 2021

Conversation

thebrianchen
Copy link

No description provided.

@thebrianchen thebrianchen requested review from a team as code owners March 16, 2021 21:53
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Mar 16, 2021
@thebrianchen thebrianchen self-assigned this Mar 16, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2021
@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #1451 (616133a) into master (fee8a04) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
- Coverage   98.22%   98.21%   -0.02%     
==========================================
  Files          32       32              
  Lines       19565    19567       +2     
  Branches     1283     1285       +2     
==========================================
  Hits        19217    19217              
- Misses        344      346       +2     
  Partials        4        4              
Impacted Files Coverage Δ
dev/src/bulk-writer.ts 100.00% <100.00%> (ø)
dev/src/rate-limiter.ts 98.85% <0.00%> (-1.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fee8a04...616133a. Read the comment docs.

const underRateLimit = this._rateLimiter.tryMakeRequest(batch._opCount);
if (underRateLimit) {
await batch.bulkCommit({requestTag: tag});
if (flush) this._scheduleCurrentBatch(flush);
Copy link
Contributor

Choose a reason for hiding this comment

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

This codeflow still confused me, but I now know why. This sound like it is trying to re-send the same batch again. Should we call this _scheduleNextBatch()? FWIW, I think this function could also just be called _sendBatch() since it takes the batch as an argument now.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that makes more sense. Renamed.

@thebrianchen thebrianchen merged commit 3a50f8b into master Mar 16, 2021
@thebrianchen thebrianchen deleted the bc/bulk-limiter branch March 16, 2021 23:31
gcf-owl-bot bot added a commit that referenced this pull request Jun 7, 2022
Source-Link: googleapis/synthtool@cd78529
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:ddb19a6df6c1fa081bc99fb29658f306dd64668bc26f75d1353b28296f3a78e6
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 7, 2022
)

Source-Link: googleapis/synthtool@cd78529
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:ddb19a6df6c1fa081bc99fb29658f306dd64668bc26f75d1353b28296f3a78e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants