Allow to set LMDB map size in convert-db script

rdelaage commented

2024-01-17 19:50:17 +00:00

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

continuous-integration/drone/pr Build is passing Details

55b170c65a

Allow to set LMDB map size in convert-db script

zdenek.crha commented

2024-01-17 20:32:33 +00:00

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?

rdelaage commented

2024-01-18 07:02:27 +00:00

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 commented

2024-01-18 08:08:16 +00:00

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?

lx commented

2024-01-18 10:22:03 +00:00

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.

zdenek.crha commented

2024-01-18 11:09:25 +00:00

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.