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

featrue: Include go-ds-pebble as built-in plugin #10367

Closed
wants to merge 1 commit into from

Conversation

guojidan
Copy link

closed: #10347

I have tested it locally, It seems to be working properly 🤔

@guojidan guojidan requested a review from a team as a code owner March 12, 2024 09:23
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 for taking a stab at this @guojidan.
Are you able to add tests in similar to ones we have for badgerds in

  • test/sharness/t0060-daemon.sh
  • test/sharness/t0025-datastores.sh

?

@@ -23,6 +23,10 @@ The legacy subset of the Kubo RPC that was available via the Gateway port and wa

If you have a legacy software that relies on this behavior, and want to expose parts of `/api/v0` next to `/ipfs`, use reverse-proxy in front of Kubo to mount both Gateway and RPC on the same port. NOTE: exposing RPC to the internet comes with security risk: make sure to specify access control via [API.Authorizations](https://github.com/ipfs/kubo/blob/master/docs/config.md#apiauthorizations).

#### DataStore: Include pebble as built-in plugin

support pebble in datadstore, same like leveldb. You can read more in <https://github.com/ipfs/kubo/issues/10347>.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
support pebble in datadstore, same like leveldb. You can read more in <https://github.com/ipfs/kubo/issues/10347>.
Support pebble in datastore, same like leveldb. You can read more in <https://github.com/ipfs/kubo/issues/10347>.

Comment on lines +54 to +63
## pebbleds

Uses [pebble](https://github.com/cockroachdb/pebble) as a key value store

```json
{
"type": "pebbleds",
"path": "<location of pebble inside repo>"
}
```
Copy link
Member

Choose a reason for hiding this comment

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

@hsanjuan I know pebble is used by default in IPFS Cluster, and there is a lot of custom configuration in https://github.com/ipfs-cluster/ipfs-cluster/blob/v1.0.8/datastore/pebble/config.go#L25-L70

Two questions:

  1. Does it make sense to expose them all via config here (as optional, use only when set)
  2. For implicit defaults, should we use defaults from pebble, or ones from ipfs-cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lidel it makes sense to expose them, to be honest. Why not? I would expose them explicitally like cluster does. It is the only way to adjust things to usage, unlike the current badger-datastore defaults.

For implicit defaults, you need to look into what Kubo does. Kubo has its own block-cache, etc. so might as well disable/reduce pebble's cache, for example. Also need to adapt levels and granularity to the average 256K default block size.

@guojidan
Copy link
Author

I will do this in the next few days

@lidel lidel added the need/author-input Needs input from the original author label Mar 26, 2024
@gammazero gammazero assigned gammazero and unassigned gammazero Jul 30, 2024
@lidel
Copy link
Member

lidel commented Jul 30, 2024

Triage question: @guojidan will you have interest and time to work on this, or is it ok for someone else to pick up the pebble work?

@lidel lidel assigned lidel and gammazero and unassigned lidel Jul 30, 2024
@guojidan
Copy link
Author

Triage question: @guojidan will you have interest and time to work on this, or is it ok for someone else to pick up the pebble work?

I am so sorry. I have no time to do this work. If someone is interested in this, please feel free to take it. Thanks.

@lidel
Copy link
Member

lidel commented Aug 6, 2024

@guojidan no problem! @gammazero will pick this up after we are done with more pressing stuff.

closing this PR, we will continue in #10347 + new one.

@lidel lidel closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support pebble
4 participants