Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove badger, leveldb dependencies #286

Merged
merged 4 commits into from
May 10, 2023

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Apr 26, 2023

Boxo should not bring in such external dependencies only because some benchmarks that no one looks at anymore decided to use those deps. They have too many strings attached (i.e. badger doesn't build on some platforms etc).

@hsanjuan hsanjuan requested review from lidel, hacdias and a team as code owners April 26, 2023 13:16
@hsanjuan hsanjuan changed the title Remove badger leveldb deps Remove badger, leveldb dependencies Apr 26, 2023
Boxo should not bring in such external dependencies only because some
benchmarks that no one looks at anymore decided to use those deps. They have
too many strings attached (i.e. badger doesn't build on some platforms etc).
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #286 (6d06dfd) into main (268cad8) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   47.88%   47.85%   -0.03%     
==========================================
  Files         273      273              
  Lines       33186    33186              
==========================================
- Hits        15890    15881       -9     
- Misses      15616    15627      +11     
+ Partials     1680     1678       -2     

see 10 files with indirect coverage changes

@lidel lidel changed the title Remove badger, leveldb dependencies refactor: remove badger, leveldb dependencies Apr 27, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! removing these deps sounds sensible

(:thought_balloon: we've pulled in a lot of things when we migrated repos, and i suspect we have a lot of deps that are only used in tests. these are more tricky to cleanup -- we run tests, but have no time to refactor to use less deps. probably something that needs to happen organically over time)

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may looks like we just use it as a KV store but this is wrong.
In multiple places we incorrectly implement a graph dB by listing and filtering keys.
This data store implement this by listing everything all the keys using go's map range iteration syntax which is not fast and O(N) based on the full size, not bucketed.
Also dht server store GiBs of data which is nice to do on disk.

pinning/pinner/dspinner/pin_test.go Show resolved Hide resolved
@guseggert
Copy link
Contributor

Are you having a concrete issue w/ this?

In general, consumers won't get this dependency in their build list--even if they directly use the dspinner package--since it's a test dependency.

@Jorropo Jorropo dismissed their stale review April 29, 2023 00:44

It's my own review lol.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented May 1, 2023

Are you having a concrete issue w/ this?

In general, consumers won't get this dependency in their build list--even if they directly use the dspinner package--since it's a test dependency.

These rather superfluous tests make Badger appear in go.mod. That automatically directs dependency resolution when nothing overrides it, and I think can cause conflicts sometimes or cause importing old versions of things. This comes after ipfs/kubo#9839, although I think this could stay just fine (because it's just tests as you say). I just thought there's no need for boxo to pull in these deps at all in its dependency tree, particularly given the low value of doing so (it's not even tests, it's benchmarks).

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable change. I'm not very versed in how Go dependency management works, but assuming what @hsanjuan is true, this is likely a good change and should not affect anything as we're not really removing any tests.

@Jorropo Jorropo merged commit 50e384f into main May 10, 2023
@Jorropo Jorropo deleted the remove-badger-leveldb-deps branch May 10, 2023 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants