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

feat(caddy): persist replay cache across config reloads #212

Merged
merged 221 commits into from
Oct 7, 2024

Conversation

sbruens
Copy link

@sbruens sbruens commented Sep 18, 2024

As the replay history config could be changed, this introduces a way to resize the ReplayCache in case the capacity needs to shrink.

sbruens added 30 commits May 31, 2024 17:03
@sbruens sbruens changed the title feat: persist replay cache across config reloads feat(caddy): persist replay cache across config reloads Sep 18, 2024
@sbruens sbruens marked this pull request as ready for review September 18, 2024 21:09
@sbruens sbruens requested a review from a team as a code owner September 18, 2024 21:09
caddy/app.go Outdated Show resolved Hide resolved
caddy/app.go Outdated Show resolved Hide resolved
Base automatically changed from sbruens/caddy to master September 23, 2024 18:38
@sbruens sbruens force-pushed the sbruens/replaycache branch 2 times, most recently from 43318e7 to 9de7a6a Compare October 7, 2024 19:11
@sbruens sbruens requested a review from fortuna October 7, 2024 19:12
@sbruens
Copy link
Author

sbruens commented Oct 7, 2024

Picking this up again. PTAL @fortuna

Copy link

@fortuna fortuna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

// archive.
func (c *ReplayCache) Resize(capacity int) {
if capacity > MaxCapacity {
panic("ReplayCache capacity would result in too many false positives")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please return an error and handle it instead of crashing the server. Config updates shouldn't crash the server (perhaps log error and keep the old config).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to mimic the constructor's behavior. Do you want me to update it there as well or leave that one alone, since that's more likely to be called on startup by a caller?

service/replay_test.go Show resolved Hide resolved
service/replay.go Outdated Show resolved Hide resolved
@sbruens sbruens merged commit 3c24817 into master Oct 7, 2024
5 checks passed
@sbruens sbruens deleted the sbruens/replaycache branch October 7, 2024 21:31
@@ -92,11 +93,25 @@ func (c *ReplayCache) Add(id string, salt []byte) bool {
return false
}
_, inArchive := c.archive[hash]
if len(c.active) == c.capacity {
if len(c.active) >= c.capacity {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fortuna I already merged a bit too fast, but just wanted to point out this specific change I made as part of not actively shrinking the maps in the Resize() method. This is needed, otherwise we'll never shrink maps that were resized down.

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