Move badger, tests and benchmarks to sub-modules #28

muXxer · 2023-11-20T08:13:27Z

Hello! 👋

I don't know if that is the best way to do it, and if you like the change at all, but this PR is trying to get rid of the badger dependency for users that don't need the badger KVStore, by moving the badger implementation to a new module called "github.com/pokt-network/smt/kvstore".

This dependency was introduced in #19

h5law

Hi @muXxer 👋🏻 Love to see your interest in the SMT!

Before I do a full review I have a few higher level requests. I like the idea of removing the badgerDB dependency by introducing a submodule. However I think moving the benchmarks and tests into submodules isn't the right move.

This approach forces a lot of methods to be exposed that we don't need to be exposed.

I think re-introducing the SimpleMap and having the MapStore conform to the same interface we use with badger would be ideal. The methods on the interface related to backups and restoration would be no-op but this would allow users to use either a simple go map type or the badger db for a more production use case.

So what do you think of moving the benchmarks and tests back into the main package, unexporting the methods that were changed and replacing the tests usage of badger with the MapStore (this can be copy pasted from older versions probably with little changes)? This way we can still keep unexposed numerous methods that don't need to be exposed publicly, and also remove the dependency on badger for those that dont want it, while still keeping a builtin kvstore.

Olshansk · 2023-11-22T21:37:26Z

Firstly, love the idea of removing the explicit dependency on badger as I think it'd be nice to experiment & explore with stores likes boltdb or rocksdb as well.

+1 to @h5law's suggestion, so let us know what you think.

@muXxer Also, we have other plans/ideas for features we want to add (e.g. making the smt support k-ary children instead of just binary children) in the future. Feel free to give us a heads up to make sure that we don't accidentally duplicate your work.

muXxer · 2023-11-23T08:25:10Z

Hi @h5law and @Olshansk,
sorry for the late response, I didn't see the github notification.

@h5law to you suggestion, I actually thought exactly the same when I did these changes. Exporting all the functions just because of the tests and benchmarks is quite ugly, and a map implementation would be nicer indeed. But I was in a rush when I did that, and I thought it's good to get some feedback first. 😅

I can try to do it the way you said 👍

Actually, we already have another KVStore interface in our codebase, which you maybe could use instead if you want. 😅
We have support for a map based KVStore there as well, and RocksDB. In the past we also had badger/v3 and bbolt, but we removed those because RocksDB is by fast the fastest and most reliable so far. https://github.com/iotaledger/hive.go/tree/develop/kvstore

muXxer · 2023-11-23T17:09:01Z

@h5law done!

I reverted all exposures of the internal smt methods.

Some things to consider:

the SimpleMap has some weird behaviours because I adapted it to the checks done in kvstore_test.go, but those come from weird badger behaviour in the first place... (like checking for key size during deletion etc)
you need to remove the replace directive and the magic hash in the badger/go.mod, I can't do this because I can't push to the original upstream repo, so I can't reference an older commit of myself (maybe directly after the PR gets merged?)
I deactivated the restore/backup test for SimpleMap 🤷
I'm not sure if the badger tests get executed by your CI 🏃

…dependencies

h5law · 2023-11-23T17:46:44Z

kvstore/badger/badger.go


 type badgerKVStore struct {
 	db          *badger.DB
 	last_backup uint64 // timestamp of the most recent backup
 }

+var badgerToSMTErrorsMap = map[error]error{


Can we add some quick comments explaining this map and the conversion function in the godoc format

h5law · 2023-11-23T17:49:03Z

kvstore/badger/go.mod

+
+go 1.20
+
+replace github.com/pokt-network/smt => ../../


Suggested change

replace github.com/pokt-network/smt => ../../

Saw your comment, if we remove this, now I have enabled CI for this PR lets see what happens. I can always prepare a follow up PR that is based off this that we ultimately merge in instead

I would suggest to remove the line and update to the correct commit in your follow up PR, which should be merged directly after this one.

h5law · 2023-11-23T17:51:18Z

kvstore/simplemap/simplemap.go

+	"encoding/hex"
+	"fmt"
+	"io"
+	"slices"


This requires go 1.21 - might as well update the repo, you'll have to adjust the workflow files in the .github directory and update the note in the readme. CI should catch this stuff now

Updated to 1.21 👍

h5law · 2023-11-23T17:52:18Z

kvstore/simplemap/simplemap.go

+	Key []byte
+}
+
+func (e *InvalidKeyError) Error() string {


Can we add some godoc comments similar to the badger KVStore ones (copy paste will probably be enough with minor changes) to all the functions and structs in here.

h5law · 2023-11-23T17:52:57Z

kvstore/simplemap/simplemap_test.go

+	"testing"
+)
+
+func TestSimpleMap(t *testing.T) {


Do we need this file? Is this not covered in simplemap/kvstore_test.go?

I kept it because it was part of the former removed SimpleMap, I only adapted it to this version.
But I guess you are right, the kvstore_test should already cover that. I removed the file again.

h5law

@muXxer Overall love the PR! Thanks for addresses the comments before I did a review.

Left some comments, mostly around godoc comments - I think its better to have them even on non-exported functions just for readability.

Otherwise love the direction this is going in. As @Olshansk one of my goals with this repo is to expand the maps. I will have a look into your KVStore interface and see what I can do!

To give some context I want to make this a k-ary tree eventually where the user can decide which k they desire. Having multiple maps as submodules like how you have done with our badger map is going to be a huge benefit. I will work on adding support for more and more over time.

CI should run on these commits now so lets see if removing your "go mod magic" works as is. However if not, I will open a PR after the comments have been addressed here and we can merge that one in, but lets give it a go first.

If you have any other requests or PRs please open issues/PRs and we can work on them! Want to see this tree get some more adoption in the industry!

Olshansk · 2023-11-24T21:54:29Z

@h5law Thanks for diving into the review. I'll take a look after the first round is addressed.

@muXxer Excited to give RocksDB a shot :)

Olshansk · 2023-12-14T21:21:30Z

@muXxer Wanted to bump and see if you're still interesting in pushing forward with this PR?

h5law · 2023-12-15T14:52:26Z

@muXxer I have been putting some thought into this and can tackle it after Christmas if there isn't a rush on your end.

…dependencies

muXxer · 2023-12-22T13:51:59Z

@h5law @Olshansk sorry that this took so long, I was pretty busy and then I almost forgot about this 😅
Hope the changes are ok now.

h5law · 2023-12-22T15:01:50Z

@muXxer it seems you may not be able to tackle this one so check out #33 and see if this suits your needs better

Olshansk · 2024-01-03T22:56:10Z

Closing this out now that #33 and #34 were merged in.

Move badger, tests and benchmarks to sub-modules

a738f8b

reviewpad bot added large Pull request is large waiting-for-review This PR is currently waiting to be reviewed labels Nov 20, 2023

Remove go.mod replace

93a6c62

h5law requested changes Nov 20, 2023

View reviewed changes

muXxer force-pushed the chore/remove-dependencies branch 8 times, most recently from f372fc5 to 84d8fcc Compare November 23, 2023 17:00

Implement SimpleMap

35cb503

muXxer force-pushed the chore/remove-dependencies branch from 84d8fcc to 35cb503 Compare November 23, 2023 17:02

Merge branch 'main' of github.com:pokt-network/smt into chore/remove-…

86f34b5

…dependencies

muXxer requested a review from h5law November 23, 2023 17:14

h5law reviewed Nov 23, 2023

View reviewed changes

h5law requested changes Nov 23, 2023

View reviewed changes

Merge branch 'main' of github.com:pokt-network/smt into chore/remove-…

17e1681

…dependencies

muXxer added 4 commits December 22, 2023 14:38

Update go version to 1.21

3fca8a9

Add more comments

8374e5a

Remove unused InvalidKeyError

bd0c6a7

Remove unnecessary test file

981fb8e

h5law mentioned this pull request Dec 22, 2023

[Enhancement] Reintroduce the MapStore interface and SimpleMap and make the Badger a submodules #33

Merged

13 tasks

Olshansk closed this Jan 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move badger, tests and benchmarks to sub-modules #28

Move badger, tests and benchmarks to sub-modules #28

muXxer commented Nov 20, 2023 •

edited

Loading

h5law left a comment

Olshansk commented Nov 22, 2023

muXxer commented Nov 23, 2023

muXxer commented Nov 23, 2023 •

edited

Loading

h5law Nov 23, 2023

h5law Nov 23, 2023

muXxer Dec 22, 2023

h5law Nov 23, 2023

muXxer Dec 22, 2023

h5law Nov 23, 2023

h5law Nov 23, 2023

muXxer Dec 22, 2023

h5law left a comment

Olshansk commented Nov 24, 2023

Olshansk commented Dec 14, 2023

h5law commented Dec 15, 2023

muXxer commented Dec 22, 2023

h5law commented Dec 22, 2023

Olshansk commented Jan 3, 2024

Move badger, tests and benchmarks to sub-modules #28

Move badger, tests and benchmarks to sub-modules #28

Conversation

muXxer commented Nov 20, 2023 • edited Loading

h5law left a comment

Choose a reason for hiding this comment

Olshansk commented Nov 22, 2023

muXxer commented Nov 23, 2023

muXxer commented Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h5law left a comment

Choose a reason for hiding this comment

Olshansk commented Nov 24, 2023

Olshansk commented Dec 14, 2023

h5law commented Dec 15, 2023

muXxer commented Dec 22, 2023

h5law commented Dec 22, 2023

Olshansk commented Jan 3, 2024

muXxer commented Nov 20, 2023 •

edited

Loading

muXxer commented Nov 23, 2023 •

edited

Loading