convert_db: allow LMDB map size override #691

Merged
lx merged 4 commits from zdenek.crha/garage:convert_db_lmdb_map_size into main 2024-01-24 08:19:45 +00:00
Contributor

Fixes #684

Not sure if it will ever be needed, but I tried to structure the code so that it is easy to add more optional overrides for LMDB or other databases later.

Fixes #684 Not sure if it will ever be needed, but I tried to structure the code so that it is easy to add more optional overrides for LMDB or other databases later.
zdenek.crha added 1 commit 2024-01-17 20:26:04 +00:00
convert_db: allow LMDB map size override
All checks were successful
continuous-integration/drone/pr Build is passing
8527dd87cc
zdenek.crha added 1 commit 2024-01-18 17:09:26 +00:00
convert_db: prevent conversion between same input/output engine
All checks were successful
continuous-integration/drone/pr Build is passing
4b54e053df
Use optional DB open overrides for both input and output database.

Duplicating the same override flag for input/output would result in too
many, too long flags. It would be too costly for very rare edge-case
where converting between same DB engine, just with different flags.

Because overrides flags for different engines are disjoint and we are
preventing conversion between same input/ouput DB engine, we can have
only one set.

The override flag will be passed either to input or output, based on
engine type it belongs to. It will never be passed to both of them and
cause unwelcome surprise to user.
Author
Contributor

I have added prevention of using same DB engine type on both input and output to allow using overrides for both input and output database (discussed in #690)

The DB engine type has now own enum with parsing/formatting support to make it easier to compare DB engines with multiple aliases.

There are two points I'm not completely sure about:

  1. I have added the Engine enum into db module on assumption it is generic enough and may be useful to have it there. If it is not correct, let me know and I will move it to cli::convert_db module.

  2. I have considered making Engine a non-exhaustive enum. With multi engine support, it seemed like different code-paths may decide to support/not support some engines and non-exhaustive enum would be good reminder to stop and think about that.

It would make sense in convert-db as engine support in garage may not automatically mean support for format conversion. But I don't know enough about rest of the codebase to be sure it makes sense there too. So in the end, I did not do it.

I have added prevention of using same DB engine type on both input and output to allow using overrides for both input and output database (discussed in #690) The DB engine type has now own `enum` with parsing/formatting support to make it easier to compare DB engines with multiple aliases. There are two points I'm not completely sure about: 1) I have added the `Engine` `enum` into `db` module on assumption it is generic enough and may be useful to have it there. If it is not correct, let me know and I will move it to `cli::convert_db` module. 2) I have considered making `Engine` a non-exhaustive `enum`. With multi engine support, it seemed like different code-paths may decide to support/not support some engines and non-exhaustive enum would be good reminder to stop and think about that. It would make sense in `convert-db` as engine support in garage may not automatically mean support for format conversion. But I don't know enough about rest of the codebase to be sure it makes sense there too. So in the end, I did not do it.
Owner

I have added the Engine enum into db module on assumption it is generic enough and may be useful to have it there. If it is not correct, let me know and I will move it to cli::convert_db module.

Sure, LGTM. Ideally we would also use that enum for parsing the config file so that this code can be refactored.

I have considered making Engine a non-exhaustive enum. With multi engine support, it seemed like different code-paths may decide to support/not support some engines and non-exhaustive enum would be good reminder to stop and think about that.

It would make sense in convert-db as engine support in garage may not automatically mean support for format conversion. But I don't know enough about rest of the codebase to be sure it makes sense there too. So in the end, I did not do it.

I think it is not necessary to make it non-exhaustive, we can safely assume that if support is compiled for a certain DB engine, then all features (including convert-db) will be supported.

> I have added the Engine enum into db module on assumption it is generic enough and may be useful to have it there. If it is not correct, let me know and I will move it to cli::convert_db module. Sure, LGTM. Ideally we would also use that enum for parsing the config file so that [this code](https://git.deuxfleurs.fr/Deuxfleurs/garage/src/tag/v0.9.1/src/model/garage.rs#L114) can be refactored. > I have considered making Engine a non-exhaustive enum. With multi engine support, it seemed like different code-paths may decide to support/not support some engines and non-exhaustive enum would be good reminder to stop and think about that. > > It would make sense in convert-db as engine support in garage may not automatically mean support for format conversion. But I don't know enough about rest of the codebase to be sure it makes sense there too. So in the end, I did not do it. I think it is not necessary to make it non-exhaustive, we can safely assume that if support is compiled for a certain DB engine, then all features (including convert-db) will be supported.
Author
Contributor

@lx Good and thanks for review.

I can have a look at refactoring the config file parsing code too. I would prefer to do it in different PR though, to not block this one.

@lx Good and thanks for review. I can have a look at refactoring the config file parsing code too. I would prefer to do it in different PR though, to not block this one.
lx requested changes 2024-01-22 13:31:01 +00:00
lx left a comment
Owner

LGTM, we can merge this part first. Just two minor changes since the db parameters are no longer specific to the output db

LGTM, we can merge this part first. Just two minor changes since the db parameters are no longer specific to the output db
@ -25,0 +24,4 @@
output_engine: Engine,
#[structopt(flatten)]
output_open: OpenDbOpt,
Owner

This is no longer specific to the output db, so maybe rename it db_open ?

This is no longer specific to the output db, so maybe rename it `db_open` ?
zdenek.crha marked this conversation as resolved
@ -25,0 +39,4 @@
#[cfg(feature = "lmdb")]
#[derive(StructOpt, Debug, Default)]
pub struct OpenLmdbOpt {
/// Output LMDB map size override
Owner

Similarly, comment should be updated to say that it applies to either input or output db

Similarly, comment should be updated to say that it applies to either input or output db
zdenek.crha marked this conversation as resolved
zdenek.crha added 1 commit 2024-01-22 16:52:57 +00:00
convert_db: cleanup naming and comments for open overrides
All checks were successful
continuous-integration/drone/pr Build is passing
74e72fc996
Author
Contributor

@lx I have discovered that the Engine enum causes compilation issues when garage_db is built with --no-default-features. It is due to Engine enum having no variant when no db engine feature is enabled and therefore match expression in Display trait having no branch.

This is not an issue when building whole garage, but cause compilation failures when trying to run tests on crates that rely on garage_db and do not specify database engine feature (because they don't need it). One such example is garage_util crate.

Right now I see several options:

  1. Ensure that at least one db engine feature is always selected via macros. That sounds like overkill.
  2. Having Unknown(String) variant that catches everything except engines with compiled support.
  3. Having Engine variants always compiled in, no matter which features are enabled.

I really like the no.3 as it would allows us to distinguish between invalid engine and valid engine without compiled support. I will try to implement it tomorrow (unless you prefer other variant).

BTW, what is your preference regarding review fixes? Add them as another commit or fixup the change to relevant commit?

@lx I have discovered that the `Engine` enum causes compilation issues when `garage_db` is built with `--no-default-features`. It is due to `Engine` enum having no variant when no db engine feature is enabled and therefore `match` expression in Display trait having no branch. This is not an issue when building whole `garage`, but cause compilation failures when trying to run tests on crates that rely on `garage_db` and do not specify database engine feature (because they don't need it). One such example is `garage_util` crate. Right now I see several options: 1) Ensure that at least one db engine feature is always selected via macros. That sounds like overkill. 2) Having `Unknown(String)` variant that catches everything except engines with compiled support. 3) Having `Engine` variants always compiled in, no matter which features are enabled. I really like the no.3 as it would allows us to distinguish between invalid engine and valid engine without compiled support. I will try to implement it tomorrow (unless you prefer other variant). BTW, what is your preference regarding review fixes? Add them as another commit or fixup the change to relevant commit?
Owner

I'm surprised that an enum with no variants causes a compilation issue, I thought it would just mean "this code is unreachable" for the compiler. I think it's a known practice to use an empty enum to make a type that cannot be instantiated.

Anyways, I'm fine with solution 3, having all variants always compiled in.

BTW, what is your preference regarding review fixes? Add them as another commit or fixup the change to relevant commit?

Both are fine to me, I rarely squash commits myself so don't feel obligated to do so.

I'm surprised that an enum with no variants causes a compilation issue, I thought it would just mean "this code is unreachable" for the compiler. I think it's a known practice to use an empty enum to make a type that cannot be instantiated. Anyways, I'm fine with solution 3, having all variants always compiled in. > BTW, what is your preference regarding review fixes? Add them as another commit or fixup the change to relevant commit? Both are fine to me, I rarely squash commits myself so don't feel obligated to do so.
zdenek.crha added 1 commit 2024-01-23 15:25:32 +00:00
make all garage_db::Engine variants un-conditional
All checks were successful
continuous-integration/drone/pr Build is passing
0eef8a69f0
Having all Engine enum variants conditional causes compilation errors
when *none* of the DB engine features is enabled. This is not an issue
for full garage build, but affects crates that use garage_db as
dependency.

Change all variants to be present at all times. It solves compilation
errors and also allows us to better differentiate between invalid DB
engine name and engine with support not compiled in current binary.
Author
Contributor

I' m surprised that an enum with no variants causes a compilation issue, I thought it would just mean "this code is unreachable" or the compiler. I think it's a known practice to use an empty enum to make a type that cannot be instantiated.

Empty enum is ok, compiles fine. The compilation error is in the Display trait impl. When no feature is enabled, then the match on &Engine expression will have no branch that makes compiler unhappy. I've made reduced example here:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=efd7b4d01098eb041b8c1de69603b760

Anyway, it is fixed now. Thanks for a patience with me.

> I' m surprised that an enum with no variants causes a compilation issue, I thought it would just mean "this code is unreachable" or the compiler. I think it's a known practice to use an empty enum to make a type that cannot be instantiated. Empty enum is ok, compiles fine. The compilation error is in the Display trait impl. When no feature is enabled, then the `match` on `&Engine` expression will have no branch that makes compiler unhappy. I've made reduced example here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=efd7b4d01098eb041b8c1de69603b760 Anyway, it is fixed now. Thanks for a patience with me.
Owner

Thanks for the work!

Thanks for the work!
lx merged commit 08a871390e into main 2024-01-24 08:19:45 +00:00
Sign in to join this conversation.
No reviewers
lx
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#691
No description provided.