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 sending empty request when batch is empty #5060

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

senegalo
Copy link
Contributor

@senegalo senegalo commented Nov 9, 2023

Description

When finishBundle is called a call to createRequest to send any remaining elements in the bundle in a single gRPC request. In some corner cases we can end up sending empty batch requests (with no elements) like in the following:

  • In the case where all the bundle can be satisfied from the cache
  • in the case when the bundle size == batch size

Basically any time we reach the finishBundle and the batch queue is empty we send an empty batch request as we are not checking in createRequest if there are elements or not.

Changes

Only call createRequest in finishBundle if there are still element in the batch queue.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #5060 (fc5dd11) into main (47b254b) will not change coverage.
The diff coverage is n/a.

❗ Current head fc5dd11 differs from pull request most recent head 4fdd54b. Consider uploading reports for the commit 4fdd54b to get more accurate results

@@           Coverage Diff           @@
##             main    #5060   +/-   ##
=======================================
  Coverage   63.10%   63.10%           
=======================================
  Files         286      286           
  Lines       10764    10764           
  Branches      790      790           
=======================================
  Hits         6793     6793           
  Misses       3971     3971           

@RustedBones RustedBones merged commit 4cd7058 into spotify:main Nov 9, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants