Add key transform functions for massive rate limit improvements #205

v-stickykeys · 2021-11-12T13:16:51Z

Without sharding in S3, it is unreasonable from a performance and cost perspective to use it as an IPFS datastore.

This PR adds customizable sharding methods, matching the implementation of go-ds-flatfs and compatible with js-datastore-core (also js-datastore-s3).

This incorporates the suggestion made by @whyrusleeping #195 (comment) to allow developers to decide the layout of their data on S3.

This changes the way the go-ds-s3 plugin stores data in s3 in an effort to drastically improve speed. Instead of storing data all in the /ipfs directory (which counts as one prefix), we're storing things in a way so each block gets its own prefix.

comma fix

* feat: add key transformation method for sharding * fix: use offset for next to last sharding

welcome · 2021-11-12T13:16:52Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

v-stickykeys · 2021-11-12T13:33:22Z

I wasn't able to test this unless the module name matched my repo name so that is one thing I will change (may need some guidance to figure out how to do this properly as I'm new to go!)

BigLep · 2021-11-12T16:21:31Z

@MichaelMure : are you able to review this?

obo20 · 2021-11-12T16:32:55Z

From conversations with @whyrusleeping , it's my understanding that the next-to-last/2 method (and other methods of filesystem sharding) were originally implemented in order to better scale linux based filesystems.

@achingbrain would you be able to share the reasons for sharding things out in the way you did in js-datastore-s3? Did you run into similar scalability issues or were you just trying to follow the existing filesystem sharding patterns of go-ipfs?

Also, @aschmahmann / @BigLep as we move towards go-ipfs 0.12.0, do any considerations need to be taken with the desire to "Switch to raw multihashes in datastores (including datastore version upgrade)" in the 0.12.0 upgrade? If possible, I'd love to make sure any possible compatibility issues are addressed pre-emptively here.

Stebalien · 2021-11-15T17:41:39Z

@obo20 see #105 and #199 (specifically the second one).

Stebalien · 2021-11-15T19:25:03Z

Oh, wait, sorry. You already know that.

For sharding blocks, "next to last" is probably fine. Although I'd personally just use a cheap hash function.

achingbrain · 2021-11-18T08:56:45Z

would you be able to share the reasons for sharding things out in the way you did in js-datastore-s3? Did you run into similar scalability issues or were you just trying to follow the existing filesystem sharding patterns of go-ipfs?

@jacobheun did the initial implementation so perhaps he can speak to the reason, but I get the feeling it was just following the principles set out by other datastore implementations.

jacobheun · 2021-11-18T11:53:07Z

@jacobheun did the initial implementation so perhaps he can speak to the reason, but I get the feeling it was just following the principles set out by other datastore implementations.

This is correct, it was just following the patterns of the other JS datastore implementations at the time (2018), it's just a lightweight interface to s3. @achingbrain I believe the sharding behavior changed in ipfs/js-datastore-s3#30, but it's not managed by the s3 datastore itself.

v-stickykeys · 2021-12-29T00:54:28Z

@MichaelMure I believe this is ready for review. PTAL when you get the chance!

v-stickykeys · 2022-01-10T15:56:08Z

@BigLep given no word so far from @MichaelMure is there someone else who might be available to review this?

To run tests multiple times against the same local s3 instance

BigLep · 2022-01-14T17:21:43Z

Similar response as in #195 (comment)

Core IPFS maintainers aren't planning to take this on as we aren't using this ourselves. We need to figure out a way to setup and denote community membership for this and other datastore repos. We're game to setup others as the maintainers here.

Maybe pair up with @obo20 and review each other's code (@obo20 has #195 open) or ask for other volunteers in the ipfs-operators Slack channel?

guseggert · 2022-02-25T22:07:59Z

s3.go

+}
+
+var KeyTransforms = map[string]func(ds.Key) string{
+	"default": func(k ds.Key) string {


Calling this "default" isn't very descriptive, how about "identity" or something like that?

guseggert · 2022-02-25T22:13:05Z

s3.go

+	"next-to-last/2": func(k ds.Key) string {
+		s := k.String()
+		offset := 1
+		start := len(s) - 2 - offset
+		return s[start:start+2] + "/" + s
+	},


I think there are some problems here

datastore.NewKey("a") -> panic datastore.NewKey("ab") -> /a//ab datastore.NewKey("abc") -> ab//abc datastore.NewKey("abcd") -> bc//abcd

Panics on keys w/ len == 1, should return an error instead

Keys w/ len == 2 have a slash in their prefix (should probably return an error instead?)

Keys w/ len > 2 have two slashes instead of one

A small unit test would be nice here.

Thanks for this @guseggert ! In practice, I am not seeing the double slash issue. Also just running through the logic I don't see how that would be possible but I'm new to Go 🙈.

However for keys of len 2 or smaller I agree this should be handled differently.
For keys of length 3 or more it still seems correct given a param of 2.

All I did was add this test to print out the results, should help you reproduce:

func TestTransforms(t *testing.T) { transform := KeyTransforms["next-to-last/2"] keys := []string{"ab", "abc", "abcd"} for _, k := range keys { fmt.Printf("datastore.NewKey(\"%s\") -> %s\n", k, transform(datastore.NewKey(k))) } }

Full test:

func TestTransforms(t *testing.T) { cases := []struct { transform string key string expectedS3Key string }{ { transform: "next-to-last/2", key: "ab", expectedS3Key: "ab/ab", }, { transform: "next-to-last/2", key: "abc", expectedS3Key: "ab/abc", }, { transform: "next-to-last/2", key: "abcd", expectedS3Key: "bc/abcd", }, } for _, c := range cases { t.Run(fmt.Sprintf("%s %s", c.transform, c.key), func(t *testing.T) { transform := KeyTransforms[c.transform] dsKey := datastore.NewKey(c.key) s3Key := transform(dsKey) if s3Key != c.expectedS3Key { t.Fatalf("expected %s, got %s", c.expectedS3Key, s3Key) } }) } }

output:

go test -v -run TestTransforms\$ . === RUN TestTransforms === RUN TestTransforms/next-to-last/2_ab s3_test.go:105: expected ab/ab, got /a//ab === RUN TestTransforms/next-to-last/2_abc s3_test.go:105: expected ab/abc, got ab//abc === RUN TestTransforms/next-to-last/2_abcd s3_test.go:105: expected bc/abcd, got bc//abcd --- FAIL: TestTransforms (0.00s) --- FAIL: TestTransforms/next-to-last/2_ab (0.00s) --- FAIL: TestTransforms/next-to-last/2_abc (0.00s) --- FAIL: TestTransforms/next-to-last/2_abcd (0.00s) FAIL FAIL github.com/3box/go-ds-s3 0.004s FAIL

guseggert · 2022-02-25T22:15:06Z

s3_test.go

+  localBucketName, localBucketNameSet := os.LookupEnv("LOCAL_BUCKET_NAME")
+  if !localBucketNameSet {
+    localBucketName = fmt.Sprintf("localbucketname%d", time.Now().UnixNano())
+	}


guseggert · 2022-02-25T22:15:40Z

s3_test.go

 		Region:         "local",
 		AccessKey:      "test",
 		SecretKey:      "testdslocal",
+		KeyTransform:   "default",


Could you add tests for the other transforms?

guseggert · 2022-02-25T22:29:53Z

plugin/main/main.go

@@ -1,7 +1,7 @@
 package main

 import (
-	plugin "github.com/ipfs/go-ds-s3/plugin"
+	plugin "github.com/3box/go-ds-s3/plugin"


Suggested change

plugin "github.com/3box/go-ds-s3/plugin"

plugin "github.com/ipfs/go-ds-s3/plugin"

guseggert · 2022-02-25T22:30:01Z

plugin/s3ds.go

@@ -3,7 +3,7 @@ package plugin
 import (
 	"fmt"

-	s3ds "github.com/ipfs/go-ds-s3"
+	s3ds "github.com/3box/go-ds-s3"


Suggested change

s3ds "github.com/3box/go-ds-s3"

s3ds "github.com/ipfs/go-ds-s3"

guseggert · 2022-02-25T22:30:38Z

plugin/s3ds_test.go

@@ -4,7 +4,7 @@ import (
 	"reflect"
 	"testing"

-	s3ds "github.com/ipfs/go-ds-s3"
+	s3ds "github.com/3box/go-ds-s3"


Suggested change

s3ds "github.com/3box/go-ds-s3"

s3ds "github.com/ipfs/go-ds-s3"

guseggert · 2022-02-25T22:38:25Z

README.md

+`next-to-last/2`
+- Shards by storing block data based on the second to last 2 characters of its key. E.g. `CX/CIQJ7IHPGOFUJT5UMXIW6CUDSNH6AVKMEOXI3UM3VLYJRZUISUMGCXQ`


Suggested change

`next-to-last/2`

- Shards by storing block data based on the second to last 2 characters of its key. E.g. `CX/CIQJ7IHPGOFUJT5UMXIW6CUDSNH6AVKMEOXI3UM3VLYJRZUISUMGCXQ`

`next-to-last/2`

- Shards by storing block data based on the second to last 2 characters of its key. E.g. `CX/CIQJ7IHPGOFUJT5UMXIW6CUDSNH6AVKMEOXI3UM3VLYJRZUISUMGCXQ`

- This is for compatibility with [js-datastore-s3](https://github.com/ipfs/js-datastore-s3)

For compatibility with js-datastore-s3 and also go-ds-flatfs. Don't think it's worth the note really. Seems most of the datastores utilize some form of sharding but next-to-last/2 happens to be the default for some.

BigLep · 2022-03-25T15:25:23Z

@v-stickykeys : are you going to incorporate the feedback? This looks like a good change that is worth landing.

BigLep · 2022-04-22T15:17:35Z

@v-stickykeys : are you going to be able to incorporate the feedback? We'd like to support getting this landed.

v-stickykeys · 2022-04-26T14:43:02Z

Hey @BigLep the short answer is yes but honestly it hasn't been the top of my priority list and is now more of a weekend project. My PR as it stands meets our needs but I do have another branch with improvements addressing the review which I can try to get wrapped and pushed up in the next week.

BigLep · 2022-06-03T15:19:10Z

@v-stickykeys : just checking in. Maintainers can take a look if you post an new revision with incorporated comments.

* wip * wip * wip * go back to 3box org

eth-limo · 2022-10-13T15:06:05Z

This would be great to have!

koxon · 2022-12-03T08:27:03Z

Hi guys.

I wrote the IPFS on AWS Fargate blog for AWS and I'm currently looking at making this S3 plugin work with the serverless architecture mentioned in the blog.

This would be a shame not to merge this.

S3 Sharding is key to making this a viable option compared to traditional drives.
With IPFS we're looking for high throughput. Low latency data access is less important.

Referencing this post from @mikeal, if we can achieve 40GB/s throughput per bucket we could run large clusters at a fraction of the cost. We can leverage S3 infrequent access or intelligent tiering to save on cost even more.

I will try this branch and I'm willing to help move this forward.

@guseggert you've submitted multiple feedback. What is your assessment of the situation?
Looks like there is minor work left to finalize this PR.

@v-stickykeys are you still around?

@BigLep can you add me as contributor to this PR?

hsanjuan · 2023-10-12T19:08:56Z

@koxon sorry that no one answered this... are you still interested in picking this up?

Right now I don't see any upsides in letting users configure the number of prefixes between next-to-last-2,3 or whatever... might as well set it to a number that 128 or a number that maximizes the usage of S3 and leave it there. Unless there is a good reason to use a worse number.

obo20 and others added 12 commits August 16, 2021 17:40

Change go-ds-s3 to utilizing prefixes

6d41994

This changes the way the go-ds-s3 plugin stores data in s3 in an effort to drastically improve speed. Instead of storing data all in the /ipfs directory (which counts as one prefix), we're storing things in a way so each block gets its own prefix.

bug fixes for the new data changes

d905b50

make the key tranformation pluggable

d171f05

use a simple suffix config field to make text based configs easier

7b5dccf

Update s3ds.go

2466cc3

comma fix

ce9300d

comma fix

Update go.mod

dc5d2bd

feat: update to 3box module

e823c1b

feat: add key transformation method for sharding (#1)

094a6c5

* feat: add key transformation method for sharding * fix: use offset for next to last sharding

fix: use string for key of map

2950104

clean: remove dead code

ec588c5

feat: update next to last key syntax

eb4e06a

BigLep requested a review from MichaelMure November 12, 2021 16:20

v-stickykeys added 7 commits November 18, 2021 15:17

Update s3_test.go

c052864

feat: add key transform to optional field in test

e024b96

docs: add key transform to example config

fa881fc

feat: update to go-ipfs 0.11

5317384

docs: clarify config instructions

f71e6d4

docs: clarify config instructions

46a7177

Merge branch 'master' into master

f8b8a6b

docs: provide examples for sharding options

31a194d

v-stickykeys added 3 commits January 10, 2022 15:04

feat(test): randomize bucket name by default

5f32f2f

To run tests multiple times against the same local s3 instance

fix(test): use default key transform by default

2982d65

feat(test): add key transform to config parser test

f7d6fb9

BigLep mentioned this pull request Jan 14, 2022

Update go-ds-s3 plugin to utilize prefixes for massive rate limit improvements #195

Closed

lidel requested a review from guseggert February 18, 2022 16:57

guseggert reviewed Feb 25, 2022

View reviewed changes

BigLep assigned v-stickykeys Mar 25, 2022

lidel added the need/author-input Needs input from the original author label Apr 15, 2022

feat: Make it work with Kubo (#3)

0cc3de5

* wip * wip * wip * go back to 3box org

koxon mentioned this pull request Dec 4, 2022

can one IPFS instance uses multiple s3 buckets #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add key transform functions for massive rate limit improvements #205

Add key transform functions for massive rate limit improvements #205

v-stickykeys commented Nov 12, 2021 •

edited

Loading

welcome bot commented Nov 12, 2021

v-stickykeys commented Nov 12, 2021

BigLep commented Nov 12, 2021

obo20 commented Nov 12, 2021

Stebalien commented Nov 15, 2021

Stebalien commented Nov 15, 2021

achingbrain commented Nov 18, 2021

jacobheun commented Nov 18, 2021

v-stickykeys commented Dec 29, 2021

v-stickykeys commented Jan 10, 2022

BigLep commented Jan 14, 2022

guseggert Feb 25, 2022

guseggert Feb 25, 2022

v-stickykeys Mar 2, 2022

guseggert Mar 2, 2022 •

edited

Loading

guseggert Feb 25, 2022

guseggert Feb 25, 2022

guseggert Feb 25, 2022

guseggert Feb 25, 2022

guseggert Feb 25, 2022

guseggert Feb 25, 2022

v-stickykeys Mar 3, 2022

BigLep commented Mar 25, 2022

BigLep commented Apr 22, 2022

v-stickykeys commented Apr 26, 2022

BigLep commented Jun 3, 2022

eth-limo commented Oct 13, 2022

koxon commented Dec 3, 2022 •

edited

Loading

hsanjuan commented Oct 12, 2023

	plugin "github.com/3box/go-ds-s3/plugin"
	plugin "github.com/ipfs/go-ds-s3/plugin"

	s3ds "github.com/3box/go-ds-s3"
	s3ds "github.com/ipfs/go-ds-s3"

		`next-to-last/2`
		- Shards by storing block data based on the second to last 2 characters of its key. E.g. `CX/CIQJ7IHPGOFUJT5UMXIW6CUDSNH6AVKMEOXI3UM3VLYJRZUISUMGCXQ`

Add key transform functions for massive rate limit improvements #205

Are you sure you want to change the base?

Add key transform functions for massive rate limit improvements #205

Conversation

v-stickykeys commented Nov 12, 2021 • edited Loading

welcome bot commented Nov 12, 2021

v-stickykeys commented Nov 12, 2021

BigLep commented Nov 12, 2021

obo20 commented Nov 12, 2021

Stebalien commented Nov 15, 2021

Stebalien commented Nov 15, 2021

achingbrain commented Nov 18, 2021

jacobheun commented Nov 18, 2021

v-stickykeys commented Dec 29, 2021

v-stickykeys commented Jan 10, 2022

BigLep commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guseggert Mar 2, 2022 • 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

BigLep commented Mar 25, 2022

BigLep commented Apr 22, 2022

v-stickykeys commented Apr 26, 2022

BigLep commented Jun 3, 2022

eth-limo commented Oct 13, 2022

koxon commented Dec 3, 2022 • edited Loading

hsanjuan commented Oct 12, 2023

v-stickykeys commented Nov 12, 2021 •

edited

Loading

guseggert Mar 2, 2022 •

edited

Loading

koxon commented Dec 3, 2022 •

edited

Loading