convert_db: allow LMDB map size override #691
No reviewers
Labels
No labels
action
check-aws
action
discussion-needed
action
for-external-contributors
action
for-newcomers
action
more-info-needed
action
need-funding
action
triage-required
kind
correctness
kind
ideas
kind
improvement
kind
performance
kind
testing
kind
usability
kind
wrong-behavior
prio
critical
prio
low
scope
admin-api
scope
background-healing
scope
build
scope
documentation
scope
k8s
scope
layout
scope
metadata
scope
ops
scope
rpc
scope
s3-api
scope
security
scope
telemetry
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Deuxfleurs/garage#691
Loading…
Reference in a new issue
No description provided.
Delete branch "zdenek.crha/garage:convert_db_lmdb_map_size"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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
intodb
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 tocli::convert_db
module.I have considered making
Engine
a non-exhaustiveenum
. 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.Sure, LGTM. Ideally we would also use that enum for parsing the config file so that this code can be refactored.
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.
@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.
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,
This is no longer specific to the output db, so maybe rename it
db_open
?@ -25,0 +39,4 @@
#[cfg(feature = "lmdb")]
#[derive(StructOpt, Debug, Default)]
pub struct OpenLmdbOpt {
/// Output LMDB map size override
Similarly, comment should be updated to say that it applies to either input or output db
@lx I have discovered that the
Engine
enum causes compilation issues whengarage_db
is built with--no-default-features
. It is due toEngine
enum having no variant when no db engine feature is enabled and thereforematch
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 ongarage_db
and do not specify database engine feature (because they don't need it). One such example isgarage_util
crate.Right now I see several options:
Unknown(String)
variant that catches everything except engines with compiled support.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?
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.
Both are fine to me, I rarely squash commits myself so don't feel obligated to do so.
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.
Thanks for the work!
lx referenced this pull request2024-03-01 14:14:56 +00:00