receiver/compact/store/sidecar: Added a provision in the bucket config to add a `prefix` #3289

dsayan154 · 2020-10-07T13:30:18Z

This PR is to address issue #1318 .

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Added prefix to BucketConfig struct in factory.go
Wrote wrapper function prefixedBucket wrapper struct and related wrapper functions in objstore.go

Verification

Not yet verified, this is for the maintainers to review, if this is the right approach to address this issue. I am struggling to add unit tests for this, so any hints would be helpful. But, I can test it manually if the overall feedback on this approach is positive.

Signed-off-by: dsayan154 <dsayan154@gmail.com>

…dated the documentation for it. To address [issue 1318](thanos-io#1318) Signed-off-by: dsayan154 <dsayan154@gmail.com>

Signed-off-by: dsayan154 <dsayan154@gmail.com>

bwplotka

Some comments!

Amazing and clean work BTW ❤️

docs/storage.md

pkg/objstore/objstore.go

Addressing a suggestion to modify the comment. Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: dsayan154 <dsayan154@gmail.com>

kakkoyun

Looks good. Thanks ❤️

I have a couple of suggestions.

How about embedding Bucket into new struct?
Could we maybe add some end-to-end tests?
We need a changelog entry.

pkg/objstore/objstore.go

bwplotka

LGTM, just minor comments to address, then we can merge! (:

Thanks!

docs/storage.md

pkg/objstore/client/factory.go

pkg/objstore/objstore.go

…gelog entry Signed-off-by: dsayan154 <dsayan154@gmail.com>

pkg/objstore/client/factory.go

pracucci · 2020-10-19T14:11:24Z

pkg/objstore/objstore.go

+}
+
+func (pbkt *prefixedBucket) Delete(ctx context.Context, name string) (err error) {
+	pname := filepath.Join(pbkt.prefix, name)


You should juse path.Join() and not filepath.Join(). The former guarantees paths are joined with /, which is what object stores expect.

Same comment applies across the change set.

Aso the .Join() is used quite a lot. You could add an help prefixedBucket.nameWithPrefix(name string)

yeah, might be good to add a helper function but I don't know how will it reduce the redundancy of the code, as I'll have to call the helper function same number of times as I have called filepath.Join().
But I'll update the code with your suggestion. I think it will add consistency to the code.

pracucci · 2020-10-19T14:14:05Z

pkg/objstore/objstore.go

+	return pbkt.bkt.Close()
+}
+
+func (pbkt *prefixedBucket) Iter(ctx context.Context, dir string, f func(string) error) error {


Would be possible to add a unit test on this?

Working on this.

Hi @pracucci ,
I have added few unit tests for the Iter(), can you please review this? the tests are running fine locally.

pkg/objstore/objstore.go

dsayan154 · 2020-11-29T08:44:00Z

Sorry for such a late response on this PR, I was busy with office deliverables. I hope to address these comments by next weekend.

Co-authored-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: dsayan154 <dsayan154@gmail.com>

stale · 2021-02-13T07:36:20Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

ashuec90 · 2021-03-16T08:24:15Z

Getting below error while building thanos docker image after taking this PR through make docker

>> building Thanos binary in /go/bin
 >   thanos
# github.com/thanos-io/thanos/pkg/objstore
pkg/objstore/objstore.go:488:10: cannot use &prefixedBucket literal (type *prefixedBucket) as type Bucket in return argument:
	*prefixedBucket does not implement Bucket (wrong type for Iter method)
		have Iter(context.Context, string, func(string) error) error
		want Iter(context.Context, string, func(string) error, ...IterOption) error
pkg/objstore/objstore.go:493:2: cannot use pbkt (type *prefixedBucket) as type Bucket in return argument:
	*prefixedBucket does not implement Bucket (wrong type for Iter method)
		have Iter(context.Context, string, func(string) error) error
		want Iter(context.Context, string, func(string) error, ...IterOption) error
!! command failed: build -o /go/bin/thanos -ldflags -X github.com/prometheus/common/version.Version=0.20.0-dev -X github.com/prometheus/common/version.Revision=82ea6ff33a65a109c6d8b70ef3687479a9f04325 -X github.com/prometheus/common/version.Branch=added_prefix -X github.com/prometheus/common/version.BuildUser=root@237fa8b74dcf -X github.com/prometheus/common/version.BuildDate=20210316-08:19:18  -extldflags '-static' -a -tags netgo github.com/thanos-io/thanos/cmd/thanos: exit status 2
make: *** [Makefile:130: build] Error 1

sfc-gh-jburns · 2021-03-23T21:51:18Z

What would it take to get this done?

kakkoyun · 2021-04-01T11:21:44Z

CHANGELOG.md

@@ -112,6 +114,16 @@ Highlights:
 - [#3114](https://github.com/thanos-io/thanos/pull/3114) Query Frontend: Added support for Memacached cache.
  - **breaking** Renamed flag `log_queries_longer_than` to `log-queries-longer-than`.
 - [#3166](https://github.com/thanos-io/thanos/pull/3166) UIs: Added UI for passing a `storeMatch[]` parameter to queries.
+<<<<<<< HEAD


We need to clean these

stale · 2021-06-02T16:03:07Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

dsayan154 added 12 commits September 30, 2020 16:52

wrapper function created, build successful but unit test remaining

13e4f93

Signed-off-by: dsayan154 <dsayan154@gmail.com>

linting issue:fix 1

dc1e4c7

Signed-off-by: dsayan154 <dsayan154@gmail.com>

adding the docs

7f17ac4

Signed-off-by: dsayan154 <dsayan154@gmail.com>

commit after make docs

334a162

Signed-off-by: dsayan154 <dsayan154@gmail.com>

wrapper function created, build successful but unit test remaining

c11c6d8

Signed-off-by: dsayan154 <dsayan154@gmail.com>

linting issue:fix 1

3160be7

Signed-off-by: dsayan154 <dsayan154@gmail.com>

adding the docs

658137e

Signed-off-by: dsayan154 <dsayan154@gmail.com>

commit after make docs

277fbab

Signed-off-by: dsayan154 <dsayan154@gmail.com>

I added a provision to add prefix to the bucket config yaml, and up…

1344538

…dated the documentation for it. To address [issue 1318](thanos-io#1318) Signed-off-by: dsayan154 <dsayan154@gmail.com>

Changed the Iter wrapper logic

c503e08

Signed-off-by: dsayan154 <dsayan154@gmail.com>

trying to correct Iter logic to pass e2e test

29971ca

Signed-off-by: dsayan154 <dsayan154@gmail.com>

Merge remote-tracking branch 'upstream/master'

7293c9d

Signed-off-by: dsayan154 <dsayan154@gmail.com>

dsayan154 changed the title ~~objectstore: Added a provision in the bucket config to add a prefix~~ [receiver][compact][store][sidecar]: Added a provision in the bucket config to add a prefix Oct 7, 2020

bwplotka reviewed Oct 9, 2020

View reviewed changes

docs/storage.md Outdated Show resolved Hide resolved

pkg/objstore/objstore.go Show resolved Hide resolved

pkg/objstore/objstore.go Outdated Show resolved Hide resolved

bwplotka mentioned this pull request Oct 9, 2020

*: Support multi-tenancy on object storage using custom prefix #1318

Closed

Update pkg/objstore/objstore.go

d77e236

Addressing a suggestion to modify the comment. Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: dsayan154 <dsayan154@gmail.com>

dsayan154 force-pushed the master branch from 37b7f2e to d77e236 Compare October 12, 2020 09:16

kakkoyun reviewed Oct 14, 2020

View reviewed changes

pkg/objstore/objstore.go Show resolved Hide resolved

kakkoyun changed the title ~~[receiver][compact][store][sidecar]: Added a provision in the bucket config to add a prefix~~ receiver/compact/store/sidecar: Added a provision in the bucket config to add a prefix Oct 14, 2020

bwplotka approved these changes Oct 14, 2020

View reviewed changes

docs/storage.md Outdated Show resolved Hide resolved

pkg/objstore/client/factory.go Outdated Show resolved Hide resolved

pkg/objstore/objstore.go Show resolved Hide resolved

Addressing comments of @bwplotka for updating the docs and added Chan…

b654074

…gelog entry Signed-off-by: dsayan154 <dsayan154@gmail.com>

dsayan154 force-pushed the master branch from 8086ec4 to b654074 Compare October 18, 2020 18:09

pracucci reviewed Oct 19, 2020

View reviewed changes

dsayan154 mentioned this pull request Dec 1, 2020

I would like write a unit test for a function in PR(#3289). #3521

Closed

dsayan154 and others added 2 commits December 6, 2020 20:17

Update pkg/objstore/client/factory.go removed //

a5ee9ef

Co-authored-by: Marco Pracucci <marco@pracucci.com>

addressing the review comments, and adding a CHANGELOG entry

f856015

Signed-off-by: dsayan154 <dsayan154@gmail.com>

dsayan154 force-pushed the master branch from 05104b2 to f856015 Compare December 14, 2020 09:40

dsayan154 added 2 commits December 14, 2020 15:24

resolving conflicts

001465c

Signed-off-by: dsayan154 <dsayan154@gmail.com>

regenerated docs with make

608c4fc

Signed-off-by: dsayan154 <dsayan154@gmail.com>

dsayan154 force-pushed the master branch from bc9ec97 to 608c4fc Compare December 14, 2020 13:01

stale bot added the stale label Feb 13, 2021

kakkoyun removed the stale label Feb 13, 2021

Base automatically changed from master to main February 26, 2021 16:30

kakkoyun requested changes Apr 1, 2021

View reviewed changes

stale bot added the stale label Jun 2, 2021

stale bot closed this Jun 9, 2021

jademcosta mentioned this pull request May 6, 2022

sidecar/compact/store/receiver - Add the prefix option to buckets #5337

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receiver/compact/store/sidecar: Added a provision in the bucket config to add a `prefix` #3289

receiver/compact/store/sidecar: Added a provision in the bucket config to add a `prefix` #3289

dsayan154 commented Oct 7, 2020 •

edited

Loading

bwplotka left a comment

kakkoyun left a comment

bwplotka left a comment

pracucci Oct 19, 2020

pracucci Oct 19, 2020

dsayan154 Nov 29, 2020

pracucci Oct 19, 2020

dsayan154 Nov 29, 2020

dsayan154 Dec 14, 2020

dsayan154 commented Nov 29, 2020

stale bot commented Feb 13, 2021

ashuec90 commented Mar 16, 2021 •

edited

Loading

sfc-gh-jburns commented Mar 23, 2021

kakkoyun Apr 1, 2021

stale bot commented Jun 2, 2021

receiver/compact/store/sidecar: Added a provision in the bucket config to add a prefix #3289

receiver/compact/store/sidecar: Added a provision in the bucket config to add a prefix #3289

Conversation

dsayan154 commented Oct 7, 2020 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

pracucci Oct 19, 2020

Choose a reason for hiding this comment

pracucci Oct 19, 2020

Choose a reason for hiding this comment

dsayan154 Nov 29, 2020

Choose a reason for hiding this comment

pracucci Oct 19, 2020

Choose a reason for hiding this comment

dsayan154 Nov 29, 2020

Choose a reason for hiding this comment

dsayan154 Dec 14, 2020

Choose a reason for hiding this comment

dsayan154 commented Nov 29, 2020

stale bot commented Feb 13, 2021

ashuec90 commented Mar 16, 2021 • edited Loading

sfc-gh-jburns commented Mar 23, 2021

kakkoyun Apr 1, 2021

Choose a reason for hiding this comment

stale bot commented Jun 2, 2021

receiver/compact/store/sidecar: Added a provision in the bucket config to add a `prefix` #3289

receiver/compact/store/sidecar: Added a provision in the bucket config to add a `prefix` #3289

dsayan154 commented Oct 7, 2020 •

edited

Loading

ashuec90 commented Mar 16, 2021 •

edited

Loading