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(iroh-dns-server): Add evictable dns store #2994

Closed

Conversation

PaulOlteanu
Copy link
Contributor

@PaulOlteanu PaulOlteanu commented Dec 3, 2024

Description

Sort of some progress towards #2912, although for an in-memory store only. I don't think these changes can really be extended to a persistent one so it might be better to come up with a strategy that works for persistent and in-memory stores instead of this.

Breaking Changes

Notes & open questions

To be honest I'm not sure why I refactored the store to be a trait - I think it's unlikely this dns server will actually be extended by others.
I have also not tested this change at all, and I'm also not sure how the StoreType actually looks in a config file (but this is definitely something that should be configurable - a user can't use the existing in-memory store without modifying the hardcoded value and recompiling the server).

I'm not even particularly advocating for the timedmap crate here, I think we could use any implementation of a hashmap with eviction (https://crates.io/crates/delay_map, our own, etc.).
Like previously mentioned, I'm more interested in trying to figure out if we think it's worth adding something of this kind now and figuring out a solution for cleaning up the persistent store later, or if we want to figure out a solution entirely built on redb to keep the differences in the implementation small

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@@ -41,6 +45,7 @@ rustls-pemfile = { version = "2.1" }
serde = { version = "1", features = ["derive"] }
struct_iterable = "0.1.1"
strum = { version = "0.26", features = ["derive"] }
timedmap = "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a dependency on ttl_cache which has an almost identical API.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Just for reference, I know this doesn't matter right now)

@rklaehn
Copy link
Contributor

rklaehn commented Dec 3, 2024

I am not fond of adding a trait. Traits cause a lot of friction and box you in to a particular kind of implementation.

If I understand it correctly the use case for this is to be able to run the in-memory store indefinitely, in part to solve performance problems with the persistent store. There are some very low hanging fruits to improve the persistent store, we should exhaust those before we give up on persistence. See #2995

Also, we should probably just add eviction to the persistent store. See #2997

@PaulOlteanu PaulOlteanu closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants