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

Remove legacy RecordStore support #3374

Merged
merged 2 commits into from
Feb 10, 2025
Merged

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Feb 7, 2025

As you can see from usage locations, we've been using () implementation for quite some time, but farmer was still able to serve very old clients that relied on requests through Kademlia's DHT and its RecordStore implementation.

RecordStore has synchronous API and doesn't allow us to do more advanced iterative lookups, which is why we have implemented custom logic and abandoned RecordStore usage.

This moves DummyRecordStore from piece_provider module to constructor and simply prunes record store configuration option with all generics that it involved.

Code contributor checklist:

@nazar-pc nazar-pc enabled auto-merge February 7, 2025 12:33
@nazar-pc nazar-pc disabled auto-merge February 7, 2025 12:33
@nazar-pc nazar-pc enabled auto-merge February 7, 2025 12:35
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

I appreciate removing/replacing () with DummyRecordStore, I find code much more readable when I don’t have to guess what () means, and if it’s the same as the () in other parts of the code.

@nazar-pc nazar-pc added this pull request to the merge queue Feb 10, 2025
Merged via the queue into main with commit 52ca77f Feb 10, 2025
8 checks passed
@nazar-pc nazar-pc deleted the remove-legacy-record-store-support branch February 10, 2025 03:35
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