Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Compactor: Fix for #844 - Ignore object if it is the current directory #1544

Merged
merged 2 commits into from
Sep 24, 2019
Merged

Compactor: Fix for #844 - Ignore object if it is the current directory #1544

merged 2 commits into from
Sep 24, 2019

Conversation

jimbobby5
Copy link
Contributor

It seems the s3 client has the (slightly unexpected) behaviour that the ListObjects call also returns the directory itself, which causes the recursion experienced in #844.

Changes

Update bucket folder iteration logic to skip folder if it is the current folder.

Verification

Deployed this version into test environment which was exhibiting the issue in #844

@jimbobby5 jimbobby5 mentioned this pull request Sep 19, 2019
Closed
@GiedriusS
Copy link
Member

@jimbobby5 may I know what kind of S3 API implementation are you using? 🤔

@jimbobby5
Copy link
Contributor Author

I believe this is using the minio s3 client.

@GiedriusS
Copy link
Member

@jimbobby5 I am talking about the server-side part because we don't have any issue like this here. Just curious. Overall the code LGTM.

@jimbobby5
Copy link
Contributor Author

jimbobby5 commented Sep 19, 2019 •
edited
Loading

Oh sorry I misunderstood the question. Have seen this on AWS S3 as well as Quobyte.

bwplotka
bwplotka reviewed Sep 19, 2019
View reviewed changes
pkg/objstore/s3/s3.go Outdated
@@ -239,6 +239,10 @@ func (b *Bucket) Iter(ctx context.Context, dir string, f func(string) error) err
if object.Key == "" {
continue
}
// The s3 client can also return the directory itself in the ListObjects call above
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing trailing period.

bwplotka
bwplotka reviewed Sep 19, 2019
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I am not opposed by this I think there is no harm in such check, but it's not trivial how it translates to this very rare long replicated name.

Can we maybe create some quick unit test against this? (:

@jimbobby5
Add full-stop
dddfa76 
Signed-off-by: Jamie Poole <[email protected]>
GiedriusS
GiedriusS approved these changes Sep 23, 2019
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jimbobby5
Copy link
Contributor Author

I had a look at adding a test but unfortunately the minio client code isn't very mockable, so it's difficult to add a test for this without refactoring the client code.

@brancz brancz merged commit 749b378 into thanos-io:master Sep 24, 2019
@jimbobby5 jimbobby5 deleted the patch-1 branch September 24, 2019 13:16
ivan-kiselev pushed a commit to ivan-kiselev/thanos that referenced this pull request Sep 26, 2019
@jimbobby5
Compactor: Fix for thanos-io#844 - Ignore object if it is the current…
d367189 
… directory (thanos-io#1544)

* Ignore object if it is the current directory

Signed-off-by: Jamie Poole <[email protected]>

* Add full-stop

Signed-off-by: Jamie Poole <[email protected]>
ivan-kiselev pushed a commit to ivan-kiselev/thanos that referenced this pull request Sep 26, 2019
@jimbobby5
Compactor: Fix for thanos-io#844 - Ignore object if it is the current…
37ac272 
… directory (thanos-io#1544)

* Ignore object if it is the current directory

Signed-off-by: Jamie Poole <[email protected]>

* Add full-stop

Signed-off-by: Jamie Poole <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>
brancz pushed a commit that referenced this pull request Sep 26, 2019
@brancz
Compact resolution/retention docs update. (#1548)
ae45e27 
* Some updates to compact docs

Signed-off-by: Ivan Kiselev <[email protected]>

* some formatting

Signed-off-by: Ivan Kiselev <[email protected]>

* Update docs/components/compact.md

accept PR suggestions

Co-Authored-By: Bartlomiej Plotka <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add metalmatze to list of maintainers (#1547)

Signed-off-by: Matthias Loibl <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* resolve comments

Signed-off-by: Ivan Kiselev <[email protected]>

* resolve last comment

Signed-off-by: Ivan Kiselev <[email protected]>

* receive: Add liveness and readiness probe (#1537)

* Add prober to receive

Signed-off-by: Kemal Akkoyun <[email protected]>

* Add changelog entries

Signed-off-by: Kemal Akkoyun <[email protected]>

* Update README

Signed-off-by: Kemal Akkoyun <[email protected]>

* Remove default

Signed-off-by: Kemal Akkoyun <[email protected]>

* Wait hashring to be ready

Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* downsample: Add liveness and readiness probe (#1540)

* Add readiness and liveness probes for downsampler

Signed-off-by: Kemal Akkoyun <[email protected]>

* Add changelog entry

Signed-off-by: Kemal Akkoyun <[email protected]>

* Remove default

Signed-off-by: Kemal Akkoyun <[email protected]>

* Set ready

Signed-off-by: Kemal Akkoyun <[email protected]>

* Update CHANGELOG

Signed-off-by: Kemal Akkoyun <[email protected]>

* Clean CHANGELOG

Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Document the dnssrvnoa option (#1551)

Signed-off-by: Antonio Santos <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* feat store: added readiness and livenes prober (#1460)

Signed-off-by: Martin Chodur <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add Hotstar to adopters. (#1553)

It's the largest streaming service in India that does cricket and GoT
for India. They have insane scale and are using Thanos to scale their
Prometheus.

Spoke to them offline about adding the logo and will get a signoff here
too.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Fix hotstar logo in the adoptor's list (#1558)

Signed-off-by: Karthik Vijayaraju <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Fix typos, including 'fomrat' -> 'format' in tracing.config-file help text. (#1552)

Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Compactor: Fix for #844 - Ignore object if it is the current directory (#1544)

* Ignore object if it is the current directory

Signed-off-by: Jamie Poole <[email protected]>

* Add full-stop

Signed-off-by: Jamie Poole <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Adding doc explaining the importance of groups for compactor (#1555)

Signed-off-by: Leo Meira Vital <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add blank line for list (#1566)

The format of these files is wrong in the web.

Signed-off-by: dongwenjuan <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Refactor compactor constants, fix bucket column (#1561)

* compact: unify different time constants

Use downsample.* constants where possible. Move the downsampling time
ranges into constants and use them as well.

Signed-off-by: Giedrius Statkevičius <[email protected]>

* bucket: refactor column calculation into compact

Fix the column's name and name it UNTIL-DOWN because that is what it
actually shows - time until the next downsampling.

Move out the calculation into a separate function into the compact
package. Ideally we could use the retention policies in this calculation
as well but the `bucket` subcommand knows nothing about them :-(

Signed-off-by: Giedrius Statkevičius <[email protected]>

* compact: fix issues with naming

Reorder the constants and fix mistakes.

Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* remove duplicate

Signed-off-by: Ivan Kiselev <[email protected]>
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
@jimbobby5 @GiedriusS
Compactor: Fix for #844 - Ignore object if it is the current directory (
a573c9d 
#1544)

* Ignore object if it is the current directory

Signed-off-by: Jamie Poole <[email protected]>

* Add full-stop

Signed-off-by: Jamie Poole <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
@GiedriusS
Compact resolution/retention docs update. (#1548)
04efb74 
* Some updates to compact docs

Signed-off-by: Ivan Kiselev <[email protected]>

* some formatting

Signed-off-by: Ivan Kiselev <[email protected]>

* Update docs/components/compact.md

accept PR suggestions

Co-Authored-By: Bartlomiej Plotka <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add metalmatze to list of maintainers (#1547)

Signed-off-by: Matthias Loibl <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* resolve comments

Signed-off-by: Ivan Kiselev <[email protected]>

* resolve last comment

Signed-off-by: Ivan Kiselev <[email protected]>

* receive: Add liveness and readiness probe (#1537)

* Add prober to receive

Signed-off-by: Kemal Akkoyun <[email protected]>

* Add changelog entries

Signed-off-by: Kemal Akkoyun <[email protected]>

* Update README

Signed-off-by: Kemal Akkoyun <[email protected]>

* Remove default

Signed-off-by: Kemal Akkoyun <[email protected]>

* Wait hashring to be ready

Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* downsample: Add liveness and readiness probe (#1540)

* Add readiness and liveness probes for downsampler

Signed-off-by: Kemal Akkoyun <[email protected]>

* Add changelog entry

Signed-off-by: Kemal Akkoyun <[email protected]>

* Remove default

Signed-off-by: Kemal Akkoyun <[email protected]>

* Set ready

Signed-off-by: Kemal Akkoyun <[email protected]>

* Update CHANGELOG

Signed-off-by: Kemal Akkoyun <[email protected]>

* Clean CHANGELOG

Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Document the dnssrvnoa option (#1551)

Signed-off-by: Antonio Santos <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* feat store: added readiness and livenes prober (#1460)

Signed-off-by: Martin Chodur <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add Hotstar to adopters. (#1553)

It's the largest streaming service in India that does cricket and GoT
for India. They have insane scale and are using Thanos to scale their
Prometheus.

Spoke to them offline about adding the logo and will get a signoff here
too.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Fix hotstar logo in the adoptor's list (#1558)

Signed-off-by: Karthik Vijayaraju <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Fix typos, including 'fomrat' -> 'format' in tracing.config-file help text. (#1552)

Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Compactor: Fix for #844 - Ignore object if it is the current directory (#1544)

* Ignore object if it is the current directory

Signed-off-by: Jamie Poole <[email protected]>

* Add full-stop

Signed-off-by: Jamie Poole <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Adding doc explaining the importance of groups for compactor (#1555)

Signed-off-by: Leo Meira Vital <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Add blank line for list (#1566)

The format of these files is wrong in the web.

Signed-off-by: dongwenjuan <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* Refactor compactor constants, fix bucket column (#1561)

* compact: unify different time constants

Use downsample.* constants where possible. Move the downsampling time
ranges into constants and use them as well.

Signed-off-by: Giedrius Statkevičius <[email protected]>

* bucket: refactor column calculation into compact

Fix the column's name and name it UNTIL-DOWN because that is what it
actually shows - time until the next downsampling.

Move out the calculation into a separate function into the compact
package. Ideally we could use the retention policies in this calculation
as well but the `bucket` subcommand knows nothing about them :-(

Signed-off-by: Giedrius Statkevičius <[email protected]>

* compact: fix issues with naming

Reorder the constants and fix mistakes.

Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Ivan Kiselev <[email protected]>

* remove duplicate

Signed-off-by: Ivan Kiselev <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka left review comments

@povilasv povilasv povilasv approved these changes

@GiedriusS GiedriusS GiedriusS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimbobby5 @GiedriusS @bwplotka @povilasv @brancz