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

Implement multiple Backing Store using Generics #765

Closed
wants to merge 9 commits into from

Conversation

KhaledEmaraDev
Copy link
Contributor

This PR adds different Backing Database Stores for the Index Structures. This is done using generics. Each Backing Store is behind a feature flag and its dependencies only compile when this features is enabled.

@KhaledEmaraDev
Copy link
Contributor Author

I had to change the toolchain to version 1.61.0 because of redb.

@KhaledEmaraDev
Copy link
Contributor Author

KhaledEmaraDev commented Aug 19, 2022

@romanz Also, Cargo.toml feature dep: needs cargo 1.56.0 at least. That's why it's failing the CI.

@casey
Copy link

casey commented Aug 19, 2022

@cberner will definitely be interested in this!

@KhaledEmaraDev
Copy link
Contributor Author

This is still incomplete as for instance in redb. I haven't been able to get a prefix iterator so far. For instance in the test:

let items: &[&[u8]] = &[
    b"ab",
    b"abcdefgh",
    b"abcdefghj",
    b"abcdefghjk",
    b"abcdefghxyz",
    b"abcdefgi",
    b"b",
    b"c",
];

It returns everything after abcdefgh. So it's insert time order not prefix search.

I have been trying to track the prefix manually, but so far have been unsuccessful. It's very slow. However; I'm currently trying a new way of tracking prefixs.

@Kixunil
Copy link
Contributor

Kixunil commented Aug 20, 2022

I had to change the toolchain to version 1.61.0

Unfortunately, we want to support Debian stable which only has 1.48. I personally wouldn't mind having higher MSRV with opt-in feature but it needs CI modified to use 1.48 to test without it and use 1.61 with it.

@romanz romanz self-requested a review August 20, 2022 14:19
@romanz
Copy link
Owner

romanz commented Aug 20, 2022

Many thanks for the contribution!
Unfortunately, I am not available now - will review in two weeks.

Copy link
Owner

@romanz romanz left a comment

Choose a reason for hiding this comment

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

@KhaledEmaraDev
Copy link
Contributor Author

KhaledEmaraDev commented Aug 30, 2022

@romanz I rebased on master.

The CI is failing because:

  1. Audit: Because the GitHub action is running from a fork.
  2. Build: Because the redb backend has one slight problem just like I said above. It's making prefix scans wrong. I'm working on fixing it.
  3. Integration: Due to the Rust toolchain. I'll fix it.

Can I get the testnet indexing performance instead? I don't have the space right now to download mainnet.
I can try in a few days, though.

@antonilol
Copy link
Contributor

is this PR still being worked on? i think it would be useful to allow for different databases (maybe helps to optimize for hdd/ssd performance), but instead of choosing one at compile time, a command line flag (choose at runtime) would be better (if you use a precompiled release you can still choose) and use feature flags to choose which database backend get compiled in, especially useful for big dependencies or ones that bind to C code

@KhaledEmaraDev
Copy link
Contributor Author

@antonilol I think I can make to that backed-in storage backends are selectable using a CLI flag.

I think we should do this after merging this PR first. I still need to enhance the redb logic. I'll try to work on that sometime soon.

@antonilol
Copy link
Contributor

how is this pr going?

are you still working on it, and which things are still missing/planned (like the ones you named here, but maybe more)?

@romanz
Copy link
Owner

romanz commented May 24, 2024

Closing due to inactivity.

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.

None yet

5 participants