convert_db: allow LMDB map size override #691

zdenek.crha · 2024-01-17T20:26:04Z

zdenek.crha commented

2024-01-17 20:26:04 +00:00

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

continuous-integration/drone/pr Build is passing Details

8527dd87cc convert_db: allow LMDB map size override

zdenek.crha referenced this pull request

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

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

zdenek.crha added 1 commit 2024-01-18 17:09:26 +00:00

continuous-integration/drone/pr Build is passing Details

4b54e053df convert_db: prevent conversion between same input/output engine

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.

zdenek.crha commented

2024-01-18 17:21:41 +00:00

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:

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.
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.

rdelaage referenced this pull request

2024-01-21 08:03:27 +00:00

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

lx commented

2024-01-22 11:06:36 +00:00

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.

zdenek.crha commented

2024-01-22 13:13:15 +00:00

@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

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

src/garage/cli/convert_db.rs Outdated

					
				@ -25,0 +24,4 @@

					output_engine: Engine,

					#[structopt(flatten)]

					output_open: OpenDbOpt,

lx commented

2024-01-22 13:30:04 +00:00

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

src/garage/cli/convert_db.rs Outdated

					
				@ -25,0 +39,4 @@

				#[cfg(feature = "lmdb")]

				#[derive(StructOpt, Debug, Default)]

				pub struct OpenLmdbOpt {

					/// Output LMDB map size override

lx commented

2024-01-22 13:30:23 +00:00

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

continuous-integration/drone/pr Build is passing Details

74e72fc996 convert_db: cleanup naming and comments for open overrides

zdenek.crha commented

2024-01-22 18:31:34 +00:00

@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:

Ensure that at least one db engine feature is always selected via macros. That sounds like overkill.
Having Unknown(String) variant that catches everything except engines with compiled support.
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?

lx commented

2024-01-23 10:43:11 +00:00

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

continuous-integration/drone/pr Build is passing Details

0eef8a69f0 make all garage_db::Engine variants un-conditional

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.

zdenek.crha commented

2024-01-23 15:34:03 +00:00

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.

lx commented

2024-01-24 08:19:41 +00:00

Thanks for the work!

lx merged commit 08a871390e into main

2024-01-24 08:19:45 +00:00

lx referenced this issue from a commit

2024-01-24 08:19:47 +00:00

Merge pull request 'convert_db: allow LMDB map size override' (#691) from zdenek.crha/garage:convert_db_lmdb_map_size into main

~~lx referenced this pull request 2024-03-01 14:14:56 +00:00~~

Bump version to v0.9.2 #747

Sign in to join this conversation.

No reviewers