Skip to content
This repository has been archived by the owner on Aug 12, 2021. It is now read-only.

delete entries from the cache when the TTL expires #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marten-seemann
Copy link
Owner

This is a copy of grandcat#93.

Instead of saving the TTL value (an integer, measuring the time in seconds), we now save the expiry timestamp for every response we receive. A "cleanup" go routine then goes through our cache every 10s and removes entries that have expired.

@Stebalien, could you review this PR?

@marten-seemann marten-seemann force-pushed the cleanup-after-ttl2 branch 3 times, most recently from a86344a to 216fbdc Compare July 2, 2021 21:11
Copy link
Collaborator

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I would have done this slightly differently, but either way works.

@@ -177,6 +181,28 @@ func newClient(opts clientOpts) (*client, error) {
}, nil
}

var cleanupFreq = 10 * time.Second
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: can probably be on the order of minutes. But not really an issue any which way.

client.go Outdated
sentEntries = make(map[string]*ServiceEntry)
var entries map[string]*ServiceEntry
c.sentEntries = make(map[string]*ServiceEntry)
go c.cleanupSentEntries(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: consider doing this inline? There's no real reason to have another loop, and it saves us a lock & shared state.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's more than just a nit. That's a major simplification!

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit == not really necessary but maybe nice to have. But yeah, it's cleaner now.

@marten-seemann
Copy link
Owner Author

Rebased on top of #9, which (among other things) implements a clean shutdown. This fixes the race conditions we encountered earlier with this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants