-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
sidecar/compact/store/receiver - Add the prefix option to buckets #5337
Merged
Merged
Changes from all commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
de8dd26
Create prefixed bucket
jademcosta 64642db
started PrefixedBucket tests
dudaduarte 8e3e102
finish objstore tests
dudaduarte 124bc43
Simplify string removal logic
jademcosta 0e55117
Test more prefix cases on PrefixedBucket
jademcosta fffb2ef
Only use a prefixedbucket if we have a valid prefix
jademcosta eec2750
Add single unit test for prefixedBucket prefix
jademcosta d4a209f
test other prefixes on UsesPrefixTest
dudaduarte d494c9e
add remaining methods to UsesPrefixTest
dudaduarte 4161825
add prefix to docs examples
dudaduarte e6a9fb5
Simplify Iter method
jademcosta e7ae910
add prefix explanation to S3 docs
dudaduarte 6645a12
Conclusion of prefix sentence on docs
jademcosta cba5d77
Use DirDelim instead of magic string
jademcosta ba98b22
Add log when using prefixed bucket
jademcosta d4acbb7
Remove "@" from test string to make them simpler
jademcosta 5177900
fix BucketConfig Config type - back to interface
dudaduarte 14db917
add changelog
dudaduarte c30eb10
add missing checks in UsesPrefixTest
dudaduarte 776c17e
fix linter and test errors
dudaduarte 4ec141e
Add license to new files
jademcosta 6ee2638
Remove autogenerated docs
jademcosta 661b755
Remove duplicated transformation of string->[]byte
jademcosta bd05bd9
Add prefixed bucket on all e2e tests for S3
jademcosta 5c8baee
Add e2e tests using prefixed bucket to all providers
jademcosta 2c4fbc4
refactor: move validPrefix to prefixed_bucket logic
dudaduarte 5173567
Enhance the documentation about prefix.
jademcosta 435c130
Fix format
jademcosta 5c0c3cc
Add prefix entry on bucket config example
jademcosta e611858
Removing redundancies on prefix checks and tests
jademcosta 8926d2a
Remove redundant YAML unmarshal
jademcosta 39c8dbf
Remove unused parameter
jademcosta 3e04c41
Remove docs that should be auto-geneated
jademcosta e4a03da
refactor: move prefix to config root level
dudaduarte a7a9229
add auto generated docs
dudaduarte d72d645
fix changelog
dudaduarte File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ type: GCS | |
config: | ||
bucket: "" | ||
service_account: "" | ||
prefix: "" | ||
``` | ||
|
||
## Upload compacted blocks | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
package objstore | ||
|
||
import ( | ||
"context" | ||
"io" | ||
"strings" | ||
) | ||
|
||
type PrefixedBucket struct { | ||
bkt Bucket | ||
prefix string | ||
} | ||
|
||
func NewPrefixedBucket(bkt Bucket, prefix string) Bucket { | ||
if validPrefix(prefix) { | ||
return &PrefixedBucket{bkt: bkt, prefix: strings.Trim(prefix, DirDelim)} | ||
} | ||
|
||
return bkt | ||
} | ||
|
||
func validPrefix(prefix string) bool { | ||
prefix = strings.Replace(prefix, "/", "", -1) | ||
return len(prefix) > 0 | ||
} | ||
|
||
func conditionalPrefix(prefix, name string) string { | ||
if len(name) > 0 { | ||
return withPrefix(prefix, name) | ||
} | ||
|
||
return name | ||
} | ||
|
||
func withPrefix(prefix, name string) string { | ||
return prefix + DirDelim + name | ||
} | ||
|
||
func (p *PrefixedBucket) Close() error { | ||
return p.bkt.Close() | ||
} | ||
|
||
// Iter calls f for each entry in the given directory (not recursive.). The argument to f is the full | ||
// object name including the prefix of the inspected directory. | ||
// Entries are passed to function in sorted order. | ||
func (p *PrefixedBucket) Iter(ctx context.Context, dir string, f func(string) error, options ...IterOption) error { | ||
pdir := withPrefix(p.prefix, dir) | ||
|
||
return p.bkt.Iter(ctx, pdir, func(s string) error { | ||
return f(strings.TrimPrefix(s, p.prefix+DirDelim)) | ||
}, options...) | ||
} | ||
|
||
// Get returns a reader for the given object name. | ||
func (p *PrefixedBucket) Get(ctx context.Context, name string) (io.ReadCloser, error) { | ||
return p.bkt.Get(ctx, conditionalPrefix(p.prefix, name)) | ||
} | ||
|
||
// GetRange returns a new range reader for the given object name and range. | ||
func (p *PrefixedBucket) GetRange(ctx context.Context, name string, off int64, length int64) (io.ReadCloser, error) { | ||
return p.bkt.GetRange(ctx, conditionalPrefix(p.prefix, name), off, length) | ||
} | ||
|
||
// Exists checks if the given object exists in the bucket. | ||
func (p *PrefixedBucket) Exists(ctx context.Context, name string) (bool, error) { | ||
return p.bkt.Exists(ctx, conditionalPrefix(p.prefix, name)) | ||
} | ||
|
||
// IsObjNotFoundErr returns true if error means that object is not found. Relevant to Get operations. | ||
func (p *PrefixedBucket) IsObjNotFoundErr(err error) bool { | ||
return p.bkt.IsObjNotFoundErr(err) | ||
} | ||
|
||
// Attributes returns information about the specified object. | ||
func (p PrefixedBucket) Attributes(ctx context.Context, name string) (ObjectAttributes, error) { | ||
return p.bkt.Attributes(ctx, conditionalPrefix(p.prefix, name)) | ||
} | ||
|
||
// Upload the contents of the reader as an object into the bucket. | ||
// Upload should be idempotent. | ||
func (p *PrefixedBucket) Upload(ctx context.Context, name string, r io.Reader) error { | ||
return p.bkt.Upload(ctx, conditionalPrefix(p.prefix, name), r) | ||
} | ||
|
||
// Delete removes the object with the given name. | ||
// If object does not exists in the moment of deletion, Delete should throw error. | ||
func (p *PrefixedBucket) Delete(ctx context.Context, name string) error { | ||
return p.bkt.Delete(ctx, conditionalPrefix(p.prefix, name)) | ||
} | ||
|
||
// Name returns the bucket name for the provider. | ||
func (p *PrefixedBucket) Name() string { | ||
return p.bkt.Name() | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
package objstore | ||
|
||
import ( | ||
"context" | ||
"io/ioutil" | ||
"sort" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/thanos-io/thanos/pkg/testutil" | ||
) | ||
|
||
func TestPrefixedBucket_Acceptance(t *testing.T) { | ||
|
||
prefixes := []string{ | ||
"/someprefix/anotherprefix/", | ||
"someprefix/anotherprefix/", | ||
"someprefix/anotherprefix", | ||
"someprefix/", | ||
"someprefix"} | ||
|
||
for _, prefix := range prefixes { | ||
AcceptanceTest(t, NewPrefixedBucket(NewInMemBucket(), prefix)) | ||
UsesPrefixTest(t, NewInMemBucket(), prefix) | ||
} | ||
} | ||
|
||
func UsesPrefixTest(t *testing.T, bkt Bucket, prefix string) { | ||
testutil.Ok(t, bkt.Upload(context.Background(), strings.Trim(prefix, "/")+"/file1.jpg", strings.NewReader("test-data1"))) | ||
|
||
pBkt := NewPrefixedBucket(bkt, prefix) | ||
rc1, err := pBkt.Get(context.Background(), "file1.jpg") | ||
testutil.Ok(t, err) | ||
|
||
testutil.Ok(t, err) | ||
defer func() { testutil.Ok(t, rc1.Close()) }() | ||
content, err := ioutil.ReadAll(rc1) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, "test-data1", string(content)) | ||
|
||
testutil.Ok(t, pBkt.Upload(context.Background(), "file2.jpg", strings.NewReader("test-data2"))) | ||
rc2, err := bkt.Get(context.Background(), strings.Trim(prefix, "/")+"/file2.jpg") | ||
testutil.Ok(t, err) | ||
GiedriusS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
defer func() { testutil.Ok(t, rc2.Close()) }() | ||
contentUpload, err := ioutil.ReadAll(rc2) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, "test-data2", string(contentUpload)) | ||
|
||
testutil.Ok(t, pBkt.Delete(context.Background(), "file2.jpg")) | ||
_, err = bkt.Get(context.Background(), strings.Trim(prefix, "/")+"/file2.jpg") | ||
testutil.NotOk(t, err) | ||
testutil.Assert(t, pBkt.IsObjNotFoundErr(err), "expected not found error got %s", err) | ||
|
||
rc3, err := pBkt.GetRange(context.Background(), "file1.jpg", 1, 3) | ||
testutil.Ok(t, err) | ||
defer func() { testutil.Ok(t, rc3.Close()) }() | ||
content, err = ioutil.ReadAll(rc3) | ||
testutil.Ok(t, err) | ||
testutil.Equals(t, "est", string(content)) | ||
|
||
ok, err := pBkt.Exists(context.Background(), "file1.jpg") | ||
testutil.Ok(t, err) | ||
testutil.Assert(t, ok, "expected exits") | ||
jademcosta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
attrs, err := pBkt.Attributes(context.Background(), "file1.jpg") | ||
testutil.Ok(t, err) | ||
testutil.Assert(t, attrs.Size == 10, "expected size to be equal to 10") | ||
|
||
testutil.Ok(t, bkt.Upload(context.Background(), strings.Trim(prefix, "/")+"/dir/file1.jpg", strings.NewReader("test-data1"))) | ||
seen := []string{} | ||
testutil.Ok(t, pBkt.Iter(context.Background(), "", func(fn string) error { | ||
seen = append(seen, fn) | ||
return nil | ||
}, WithRecursiveIter)) | ||
expected := []string{"dir/file1.jpg", "file1.jpg"} | ||
sort.Strings(expected) | ||
sort.Strings(seen) | ||
testutil.Equals(t, expected, seen) | ||
|
||
seen = []string{} | ||
testutil.Ok(t, pBkt.Iter(context.Background(), "", func(fn string) error { | ||
seen = append(seen, fn) | ||
return nil | ||
})) | ||
expected = []string{"dir/", "file1.jpg"} | ||
sort.Strings(expected) | ||
sort.Strings(seen) | ||
testutil.Equals(t, expected, seen) | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hey @GiedriusS , I followed your suggestion here :) The only thing I don't love about bringing the prefix to the BucketConfig is that all the bucket options are inside Config 🤔
But with this change, we also keep the PrefixedBucket layer with its validations and don't take the risk to break something by changing all the Config types that BucketConfig handles, so I think it's the best alternative!
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.
👍 yeah, I think this is the best solution out of the alternatives