-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
less opaque chunk keys on fs with v12 #5291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if schema.VersionForChunk(chk) > 11 { | ||
split := strings.LastIndexByte(key, '/') | ||
encodedTail := base64Encoder(key[split+1:]) | ||
return strings.Join([]string{key[:split], encodedTail}, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we use filepath.Join()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use filepath
to build the keys when using fsObjectClient
because it converts /
to os specific separator using filepath.FromSlash
and filepath.ToSlash
. Basically, you would always use a /
when using fsObjectClient
, irrespective of os type .
If at all, we could use path.Join
with always uses /
, but it does not seem necessary since we are anyways working with a /
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it converts / to os specific separator using filepath.FromSlash and filepath.ToSlash.
That's why I thought we should use it, to have files in individual directories, also on windows.
With strings.Join()
it would result in files all in the same root directory? But on unix system it would be organized in subfolders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I get something wrong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What @chaudum said seems sound to me. The only time I can think where this would be problematic is mounting the same file system on different OS's (i.e. migrating on an NFS mounted Loki deployment), but I'm not sure that's something we can reasonably protect against. I'll merge this one as is in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just think it's weird to have different file/directory structure on Unix and Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if schema.VersionForChunk(chk) > 11 { | ||
split := strings.LastIndexByte(key, '/') | ||
encodedTail := base64Encoder(key[split+1:]) | ||
return strings.Join([]string{key[:split], encodedTail}, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use filepath
to build the keys when using fsObjectClient
because it converts /
to os specific separator using filepath.FromSlash
and filepath.ToSlash
. Basically, you would always use a /
when using fsObjectClient
, irrespective of os type .
If at all, we could use path.Join
with always uses /
, but it does not seem necessary since we are anyways working with a /
above.
This PR piggybacks on the yet-unreleased v12 schema in order to make some much needed improvements to the chunk key encoding when using the
fileystem
backend. Previously, the entire key was base64 encoded, meaning all chunks regardless of tenant were stored in the same directory. This made discoverability hard (the keys were opaque) and caused performance problems at scale (one infinitely growing directory). Now, in v12+ schemas, the filesystem will use<user>/<fingerprint>/base64Encode(<start>:<end>:<checksum>)
.Running locally will now look like: