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 cache index generic #3213

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Remove cache index generic #3213

merged 1 commit into from
Nov 6, 2024

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Nov 6, 2024

After thinking about it some more I no longer think there is any practical value to support more than 256 caches per controller/farmer. It is unlikely that it will be beneficial and if desired more controllers or RAID can be used to fit into this limit.

Removing this generic simplifies code quite and reduces memory usage on cluster controller a bit.

Code contributor checklist:

Copy link
Member

@shamil-gadelshin shamil-gadelshin left a comment

Choose a reason for hiding this comment

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

LGTM, one question: what will happen if we change the limit to u16::MAX instead? Do we know any practical limit?

@nazar-pc
Copy link
Member Author

nazar-pc commented Nov 6, 2024

It could be anything, just increases sizes of data structures and it seems like there is little to no benefit in doing so. But we can if we really need to, RAM is the biggest constraint.

@nazar-pc nazar-pc added this pull request to the merge queue Nov 6, 2024
@nazar-pc
Copy link
Member Author

nazar-pc commented Nov 6, 2024

One of the reasons I'm doing this is to move cluster-related utilities into from binary part of the crate into the library, but generics make it hard. There will likely be follow-up PRs after this.

Merged via the queue into main with commit cc5a9c3 Nov 6, 2024
8 checks passed
@nazar-pc nazar-pc deleted the remove-cache-index-generic branch November 6, 2024 10:29
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