Skip to content

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.

2 participants