Fix EvictedQueue bug with zero capacity #1151
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The EvictedQueue was checking for the length _before_ inserting, and popping extra items, then doing the insertion. In the case where the capacity is set to zero, it caused the pop operation to be a no-op on the first insert, and then insert an item anyway. This commit fixes the issue by moving the length check after the insert and popping any extra items. Fixes open-telemetry#1148
wperron
force-pushed
the
evicted-queue-zero-cap
branch
from
b1dc080 to
2581c4d
Compare
jtescher
approved these changes
Jul 14, 2023
lalitb
approved these changes
Jul 14, 2023
|
@wperron should get your lint issues resolved if you merge the changes currently on the main branch |
|
On vacation right now, will do when I get back in ~2 weeks 👍 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
=====================================
Coverage 49.4% 49.4%
=====================================
Files 164 164
Lines 20430 20445 +15
=====================================
+ Hits 10105 10120 +15
Misses 10325 10325
☔ View full report in Codecov by Sentry. |
|
All good, looks like I can merge main for you as well. |
|
@jtescher thanks, much appreciated! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The EvictedQueue was checking for the length before inserting, and popping extra items, then doing the insertion. In the case where the capacity is set to zero, it caused the pop operation to be a no-op on the first insert, and then insert an item anyway.
This commit fixes the issue by moving the length check after the insert and popping any extra items.
Fixes #1148
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes