Allow to set LMDB map size in convert-db script #690

Closed
rdelaage wants to merge 1 commit from rdelaage/garage:feat/set_lmdb_map_size_convert-db into main
First-time contributor

Fix #684.
This commit add the ability to set the LMDB map size in the convert-db script using a new flag called lmdb_map_size.
Feel free to suggest modifications, this is my first code in Rust

Fix #684. This commit add the ability to set the LMDB map size in the convert-db script using a new flag called `lmdb_map_size`. Feel free to suggest modifications, this is my first code in Rust
rdelaage added 1 commit 2024-01-17 19:50:18 +00:00
Allow to set LMDB map size in convert-db script
All checks were successful
continuous-integration/drone/pr Build is passing
55b170c65a
Contributor

@rdelaage Oh snap, I noticed your PR only after creating my own for the same issue :-)

Yours look simpler than mine, but harder to extend with more optional overrides (not sure it it will be needed/wanted).

The one thing I'm not sure about is using the LMDB map size override for opening both input and output databases. My thinking was that if converting from LMDB, the map_size override should/will already be set in config file. And therefore the convert-db command should set it only for output database. Did I miss something?

@rdelaage Oh snap, I noticed your PR only after creating my own for the same issue :-) Yours look simpler than mine, but harder to extend with more optional overrides (not sure it it will be needed/wanted). The one thing I'm not sure about is using the LMDB map size override for opening both input and output databases. My thinking was that if converting from LMDB, the map_size override should/will already be set in config file. And therefore the `convert-db` command should set it only for output database. Did I miss something?
Author
First-time contributor

@zdenek.crha Your PR seems cleaner. Don't if the convert-db script should read the configuration file for the input database, this is not the case right now. The user might want to convert a db from an other deployment and the configuration file does not reflect the database parameters.

@zdenek.crha Your PR seems cleaner. Don't if the convert-db script should read the configuration file for the input database, this is not the case right now. The user might want to convert a db from an other deployment and the configuration file does not reflect the database parameters.
Contributor

@rdelaage Db config not read now? I see. I also did not realize that lmdb might not be output db, because sqlite db is not being deprecated. You are right, user may need ability to pass overrides to input db too.

What concerns me is edge-case where both input/output db format is the same and user wants to override only one of them. Because that would require two sets of override parameters, one for input and other for output.

We can get around that by saying that input/output format must be different, as this command is for conversion and not tweaking database parameters. Then we would have only one set of override parameters. We could use one subset for opening input db and another, disjoint subset for opening the output db.

What do you think? Anything I'm still missing?

@rdelaage Db config not read now? I see. I also did not realize that lmdb might not be output db, because `sqlite` db is not being deprecated. You are right, user may need ability to pass overrides to input db too. What concerns me is edge-case where both input/output db format is the same and user wants to override only one of them. Because that would require two sets of override parameters, one for input and other for output. We can get around that by saying that input/output format *must* be different, as this command is for conversion and not tweaking database parameters. Then we would have only one set of override parameters. We could use one subset for opening input db and another, disjoint subset for opening the output db. What do you think? Anything I'm still missing?
Owner

I think the assumption that the input format and output format will be different will hold almost all the time, so it's fine to have only one set of parameters which can apply to both the input and output db.

I think the assumption that the input format and output format will be different will hold almost all the time, so it's fine to have only one set of parameters which can apply to both the input and output db.
Contributor

@lx Great!

@rdelaage If you don't mind, I would like to move discussion and changes to #691. The CLI option structs there are bit better prepared for adding multiple overrides for different formats. I think I'll have time to update it tonight.

@lx Great! @rdelaage If you don't mind, I would like to move discussion and changes to #691. The CLI option structs there are bit better prepared for adding multiple overrides for different formats. I think I'll have time to update it tonight.
Author
First-time contributor

I am closing this MR since MR #691 will fix the original issue

I am closing this MR since MR #691 will fix the original issue
rdelaage closed this pull request 2024-01-21 08:03:28 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details

Pull request closed

Sign in to join this conversation.
No description provided.