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

Rework heed to be lightweight and simpler to maintain #128

Merged
merged 57 commits into from
Jan 11, 2023
Merged

Conversation

Kerollmops
Copy link
Member

@Kerollmops Kerollmops commented Jul 6, 2022

This PR closes #127, closes #94, closes #56, closes #88, closes #40, closes #94, closes #132, closes #138, closes #95.

@Kerollmops Kerollmops added enhancement New feature or request breaking A change that is breaking the semver labels Jul 6, 2022
@Kerollmops Kerollmops force-pushed the a-new-era branch 7 times, most recently from 39f360e to 1fd8522 Compare July 6, 2022 16:09
@arthurprs
Copy link
Contributor

arthurprs commented Jul 14, 2022

I was trying this branch yesterday and it was returning MDB_PROBLEM sometimes when committing whereas the main branch was working fine. I researched some more and it seems there's something off with this branch, but I can't tell what exactly.

For example, if you run cargo run --example clear-database and then try to stat the database with mdb_stat target/clear-database.mdb you get

mdb_env_open failed, error -30793 MDB_INVALID: File is not an LMDB file

whereas it works in the main branch it works as expected

Status of Main DB
  Tree depth: 1
  Branch pages: 0
  Leaf pages: 1
  Overflow pages: 0
  Entries: 1

Looking at the hexdump of the datafile you can see

00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000010  00 00 08 00 00 00 00 00  de c0 ef be 02 00 00 00  |................|

but the main branch gives

00000000  00 00 00 00 00 00 00 00  00 00 08 00 00 00 00 00  |................|
00000010  de c0 ef be 01 00 00 00  00 00 00 00 00 00 00 00  |................|

Maybe I'm doing something stupid... but I wanted to mention in case I'm not.

Edit: testing was done using x64 linux, gcc 12.1, rustc 1.60.0 (7737e0b5c 2022-04-04

@Kerollmops
Copy link
Member Author

Hey @arthurprs,

Thank you very much for trying out this work-in-progress branch! I indeed had the same kind of issues and it looked like it was related to the fact that the folder to store the LMDB environment was already there, I fixed the whole list of tests by using a temporary directory. But it is very strange, I moved from the master to the master3 branch of LMDB, maybe there is an issue when we use nested write transactions to create databases or environments.

@Kerollmops Kerollmops force-pushed the a-new-era branch 2 times, most recently from 23979c5 to 42d2237 Compare September 15, 2022 11:48
@Kerollmops
Copy link
Member Author

Hey @arthurprs,

Thank you again for testing this new branch, and for finding this potential issue.

Unfortunately, I wasn't able to trigger the same issue when using mdb_stat on a database created with the clear-database example. I don't know if you notice but this branch is using the mdb.master3 branch of LMDB which is incompatible with the previous version I was using i.e. mdb.master. If you want to use the right mdb_stat binary you can go to the lmdb_master3_sys/lmdb/libraries/liblmdb folder and call make, the mdb_stat binary will be available in the folder.

I would be glad if you try out this branch and tell me whether your program is correctly working or is completely broken. Don't expect to be able to read a previous version of an LMDB database. Note that the API is changing a lot and I didn't document that for now, I will do it once this PR is merged.

@Kerollmops Kerollmops force-pushed the a-new-era branch 2 times, most recently from 64d8a4b to 6ef5bbe Compare September 15, 2022 13:31
@Techie-Pi
Copy link

Hi! I've seen that this PR uses doxygen-rs, and fyi I've just uploaded the crate to crates.io: https://crates.io/crates/doxygen-rs
Just in case you'd like to use that instead.

If you find any problem with the crate, feel free to open an issue or sending me a DM, I'll be more than happy to help! :)

@Kerollmops Kerollmops added this to the v0.20.0 milestone Jan 11, 2023
@Kerollmops
Copy link
Member Author

Thank you very much @Techie-Pi. I just updated the dependency to use a safer source which is crates.io 🙏

@Kerollmops Kerollmops force-pushed the a-new-era branch 2 times, most recently from 580168f to 31e4ddb Compare January 11, 2023 10:17
@Kerollmops
Copy link
Member Author

For information, I will merge this PR with the mdb.master LMDB branch and not the mdb.master3, for now. I will bring mdb.master3 and the encryption/decryption features in #134 later. It will be easier for Meilisearch/milli to first try the new version of the library without bringing the new LMDB version for now, I prefer moving step by step.

@Kerollmops Kerollmops force-pushed the a-new-era branch 2 times, most recently from 8dc5c2b to 6aa9c21 Compare January 11, 2023 10:35
@Kerollmops Kerollmops linked an issue Jan 11, 2023 that may be closed by this pull request
@Kerollmops
Copy link
Member Author

And here we are, heed v0.20.0-alpha.0 just released on crates.io 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment