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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
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
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.

Include go-ds-pebble as built-in plugin
3 participants