refactor: Remove dependency on concrete datastore implementations from db package #51
Conversation
Protects against accidental change, and allows the db config struct to be modified without impacting end users
There was a problem hiding this comment.
I really like the refactor here, and removing the tight dependency on implementation will make things easier in the future for some of our deployment environment work.
My only requested change would be to keep the options struct (in some way) accessible and passed into the NewDB construcutor. At the moment the options are pretty stright forward, but as things progress, additional opts will be added, and itll be good to just have an always accessible struct of provided options within the DB object itself.
cli/defradb/cmd/config.go
Outdated
| @@ -32,7 +32,6 @@ type BadgerOptions struct { | |||
|
|
|||
| // MemoryOptions for the memory instance of the backing datastore | |||
| type MemoryOptions struct { | |||
| Size uint64 | |||
There was a problem hiding this comment.
Also might want to keep this in, not used at the moment cus there was no way to include this within the current memory store, but as we talk through #40 itll probably prove helpful
There was a problem hiding this comment.
Ah easy to add back in as/when we need it, and if it exists without being used it just adds confusion. Although as I write this - this is a config struct and leaving it in preserves the name/structure. Will put it back in unless you say otherwise (I have no strong feelings here).
Would you be alright with an interface (of some sort, maybe named maybe interface{}) for that (probs wrapped by a concrete struct |
AndrewSisley
force-pushed
the
sisley/refactore/I50-break-badger-db-dependency
branch
from
7064a97 to
735e9eb
Compare
|
Added selected options via interface{} back onto the db struct via FIXUP commit (will squash before merging), let me know if you want the original object (via the interface) with both IM and Badger defined. Also removed the removal of the IM.Size property |
There was a problem hiding this comment.
The options looks fine for now. We might need to further investigate an options approach, but that will depend what ends up in the generalized opts stuct, and how its used.
The most pressing one I am imagining is LogLevels. Since at the moment we aren't tracking that, and everything is dumped into the logger, but only the default level (I think Info) is used.
AndrewSisley
force-pushed
the
sisley/refactore/I50-break-badger-db-dependency
branch
from
735e9eb to
57c8298
Compare
Ah okay I see - should hopefully be nice and easy to sort out when we get round to it :) I cleaned up the commits - guess you want to do the merging? |
AndrewSisley
deleted the
sisley/refactore/I50-break-badger-db-dependency
branch
…m db package (sourcenetwork#51) * Go mod tidy * Remove unhelpful comment * Use owned concrete type for cmd config Protects against accidental change, and allows the db config struct to be modified without impacting end users * Remove unhelpful code branch on db close * Move datastore init out of db package
Closes #50
Hands control over datastore implementations to consumers of the db package, removing the dependency on badger-db (and MapDatabase) from the core code.
Did some manual testing to make sure it wouldn't complain about dropping the inMemory Size option. Some public function/types definitions have changed.