Conversation
(Previously, it would *always* evict at least one record even if size was <= new_size)
Closer reading of RFC9113§4.3.1para¶4 (https://www.rfc-editor.org/rfc/rfc9113.html#section-4.3.1) suggests that we should always send a dynamic resize Fixes elixir-mint#20
Pull Request Test Coverage Report for Build d892f265ddeb6a5729eda4acb9802f149f4728fd-PR-21Details
💛 - Coveralls |
whatyouhide
reviewed
Dec 20, 2024
test/hpax/table_test.exs
Outdated
| end | ||
|
|
||
| describe "resizing" do | ||
| describe "resize/2" do |
Contributor
There was a problem hiding this comment.
Let's not change this, the tests are about the resizing flow
test/hpax/table_test.exs
Outdated
| end | ||
| end | ||
|
|
||
| describe "dynamic_resize/2" do |
Contributor
There was a problem hiding this comment.
Suggested change
| describe "dynamic_resize/2" do | |
| describe "dynamically resizing" do |
lib/hpax/table.ex
Outdated
Comment on lines
274
to
279
| table = | ||
| if new_protocol_max_table_size < table.size do | ||
| evict_to_size(table, new_protocol_max_table_size) | ||
| else | ||
| table | ||
| end |
Contributor
There was a problem hiding this comment.
This is making my brain hurt 😄 Here, we say to evict the table only if the protocol size goes below the table size. But the added clause to evict_to_size/2 is a no-op if we try to evict to a larger size. These are opposite I think, so it might make sense, but I think some comments would be helpful?
Contributor
Author
There was a problem hiding this comment.
Yeah you're right, we don't need this one. I'd had these two commits originally staged in the opposite order so didn't notice that this chunk is now obsolete. Removing.
Contributor
Author
|
Updated |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #20.
This changes our resize code to always send a dynamic table resize command, even if the table size did not change from the commanded version. This is in line with a closer / alternate read of RFC9113§4.3.1 paragraph 4.
I also fixed the evict_towards_size internal table function to handle the case where it was being asked to evict to a size larger than the table's current actual size. Previously we were always evicting at least one entry even if it wasn't necessary.