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
Ignore object if it is the current directory
03d3127 
Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
@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 <jimbobby5@yahoo.com>
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 <jimbobby5@yahoo.com>

* Add full-stop

Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
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 <jimbobby5@yahoo.com>

* Add full-stop

Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>
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 <kiselev_ivan@pm.me>

* some formatting

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Update docs/components/compact.md

accept PR suggestions

Co-Authored-By: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Add metalmatze to list of maintainers (#1547)

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* resolve comments

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* resolve last comment

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

* Add prober to receive

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add changelog entries

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update README

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Remove default

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Wait hashring to be ready

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

* Add readiness and liveness probes for downsampler

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add changelog entry

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Remove default

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Set ready

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update CHANGELOG

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Clean CHANGELOG

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Document the dnssrvnoa option (#1551)

Signed-off-by: Antonio Santos <antonio@santosvelasco.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <gouthamve@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Karthik Vijayaraju <karthik@hotstar.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <jimbobby5@yahoo.com>

* Add full-stop

Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Leo Meira Vital <leo.vital@nubank.com.br>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Add blank line for list (#1566)

The format of these files is wrong in the web.

Signed-off-by: dongwenjuan <dong.wenjuan@zte.com.cn>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <giedriuswork@gmail.com>

* 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 <giedriuswork@gmail.com>

* compact: fix issues with naming

Reorder the constants and fix mistakes.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* remove duplicate

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>
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 <jimbobby5@yahoo.com>

* Add full-stop

Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
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 <kiselev_ivan@pm.me>

* some formatting

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Update docs/components/compact.md

accept PR suggestions

Co-Authored-By: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Add metalmatze to list of maintainers (#1547)

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* resolve comments

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* resolve last comment

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

* Add prober to receive

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add changelog entries

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update README

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Remove default

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Wait hashring to be ready

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

* Add readiness and liveness probes for downsampler

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add changelog entry

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Remove default

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Set ready

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update CHANGELOG

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Clean CHANGELOG

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Document the dnssrvnoa option (#1551)

Signed-off-by: Antonio Santos <antonio@santosvelasco.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <gouthamve@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Karthik Vijayaraju <karthik@hotstar.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <jimbobby5@yahoo.com>

* Add full-stop

Signed-off-by: Jamie Poole <jimbobby5@yahoo.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

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

Signed-off-by: Leo Meira Vital <leo.vital@nubank.com.br>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* Add blank line for list (#1566)

The format of these files is wrong in the web.

Signed-off-by: dongwenjuan <dong.wenjuan@zte.com.cn>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* 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 <giedriuswork@gmail.com>

* 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 <giedriuswork@gmail.com>

* compact: fix issues with naming

Reorder the constants and fix mistakes.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>

* remove duplicate

Signed-off-by: Ivan Kiselev <kiselev_ivan@pm.me>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
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

@GiedriusS GiedriusS GiedriusS approved these changes

@povilasv povilasv povilasv 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