encode keys to datastore with base32 standard encoding #2903
Conversation
whyrusleeping
force-pushed
the
feat/dskey-encoding
branch
2 times, most recently
from
c2238af
to
bad252c
Compare
whyrusleeping
force-pushed
the
feat/dskey-encoding
branch
from
bad252c
to
6950edd
Compare
|
I'm in a bit of a conundrum. To merge this, we need to have a migration ready. Before i ship the migration i want to have really solid tests for it. Before i can write tests for the migrations, i need an easy way to install the new version. |
|
nevermind, the issue is that the docker tests are failing now. I love mutable build processes |
whyrusleeping
force-pushed
the
feat/dskey-encoding
branch
2 times, most recently
from
ac7e6d9
to
5483a75
Compare
| syncfs := !r.config.Datastore.NoSync | ||
| blocksDS, err := flatfs.New(path.Join(r.path, flatfsDirectory), 4, syncfs) | ||
| blocksDS, err := flatfs.New(path.Join(r.path, flatfsDirectory), 7, syncfs) |
There was a problem hiding this comment.
@whyrusleeping I am not 100% sure, but I think this value should be 6 to preserve the original intent. Here is why, the initial '/' should not be base32 encoded so the original 4 bytes will now be
1 + 3*8/5 = 5.8 = 6 bytes
instead of
4*8/5 = 6.4 = 7 bytes
There was a problem hiding this comment.
@kevina I don't think we were counting the initial slash before. That was (and is) getting cut off before being passed into the flatfs by the mount datastore
There was a problem hiding this comment.
The initial goal here was to split to 256 different directories, which is 16 bytes. With a prefix length of 7 here, we get 14 bits to shard by.
7 bytes of prefix gets us 6 characters (yay slash), at 5 bits per character that gives us 30 bits. The first 16 bits are taken by the Qm prefix, leaving us 14 bits or 16384 way sharding. Dropping down to 6 bytes will leave us with 25 total bits, and 9 bits after the prefix == a 512 way sharding.
Thanks for making me do the math, youre right. (and I like 512 way sharding better than 256 way sharding that we had before)
There was a problem hiding this comment.
@whyrusleeping it looks like you are correct in that the '/' is not included in the prefix count. The comment is misleading. That being said, I think 6 would be a better value in order to avoid too many directories for the average size repo. (I.e, it would be better to round down than up).
There was a problem hiding this comment.
@whyrusleeping. Our messages crossed. It looks like the prefix was not included in the prefix length before ipfs/go-datastore#39 but now it is (see my comments on the pull request). I will leave it up to you to determine the best value. (To avoid too many directories I believe a smaller value would be better.)
whyrusleeping
force-pushed
the
feat/dskey-encoding
branch
2 times, most recently
from
a7c38af
to
0805a9b
Compare
Fixes #2601 Also bump version to 0.4.3-dev License: MIT Signed-off-by: Jeromy <why@ipfs.io>
whyrusleeping
force-pushed
the
feat/dskey-encoding
branch
from
0805a9b
to
0782c4d
Compare
|
It is just docker failure. |
|
@lgierth i thought we fixed the docker failure |
|
Mergin time. Anyone who tries building from master after this merge will need to build the migrations and run them, at least until we put builds of the migrations up on dist. |
| syncfs := !r.config.Datastore.NoSync | ||
| blocksDS, err := flatfs.New(path.Join(r.path, flatfsDirectory), 4, syncfs) | ||
| // 5 bytes of prefix gives us 25 bits of freedom, 16 of which are taken by | ||
| // by the Qm prefix. Leaving us with 9 bits, or 512 way sharding |
There was a problem hiding this comment.
we'll want to go deeper later on
|
IIRC the main concern was in memory size of keys, but filesystem path size was also a factor. |
|
Sgtm thanks
|
This addresses #2601
Following this, we will need to:
License: MIT
Signed-off-by: Jeromy why@ipfs.io