-
Notifications
You must be signed in to change notification settings - Fork 440
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
Baggage improvements #2284
base: main
Are you sure you want to change the base?
Baggage improvements #2284
Conversation
9c71657
to
66cd5f7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2284 +/- ##
=======================================
- Coverage 79.6% 79.6% -0.1%
=======================================
Files 123 123
Lines 21263 21255 -8
=======================================
- Hits 16938 16926 -12
- Misses 4325 4329 +4 ☔ View full report in Codecov by Sentry. |
None | ||
if !key.as_str().is_ascii() { | ||
return None; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is the existing code, but baggage specs only allow for the printable ASCII characters. Probably, good to also check for control characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to fix that in efficient way, so I would prefer to leave this to someone more competent.
Some(occupied_entry.insert((value, metadata))) | ||
} | ||
Entry::Vacant(vacant_entry) => { | ||
if entries_count == MAX_KEY_VALUE_PAIRS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this is existing code, the limit is 64 as per the specs, while we use 180 - good to update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can updated that, also I noticed that MAX_BYTES_FOR_ONE_PAIR is also removed form spec, so I'll remove this as well.
a20d014
to
8438828
Compare
Changes
Improve few things:
BaggageExt::with_baggage
now "clone" baggage, instead of iterate+collect.Baggage::insert_with_metadata
(used everywhere) should be a bit faster, as it had redundant calculations:Baggage::iter
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes