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.

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

.*: Verify if deletion marker file exists before using it. #2369

Closed
wants to merge 2 commits into from
Closed

.*: Verify if deletion marker file exists before using it. #2369

wants to merge 2 commits into from

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Apr 2, 2020

This is an alternative to #2365. Please see the problem description there. This PR modifies ReadDeletionMark function to use Exists call before reading deletion marker file. This helps to keep thanos_objstore_bucket_operation_failures_total down, since missing deletion marker file is not really a failure.

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

This fixes problem with Get method reporting missing file
as failures (via metrics).

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany pstibrany changed the title Verify if deletion marker file exists before using it. .*: Verify if deletion marker file exists before using it. Apr 2, 2020
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
// BucketReader.Get reports missing file as a failure (via metrics),
// but since most blocks don't have this marker file, it skews metrics unnecessarily.
// By using exists first, we avoid that.
exists, err := bkt.Exists(ctx, deletionMarkFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm dubious about having to do another API call just to check if the object exists, when we can get the same information from Get(). I personally believe #2365 works better (my take here).

Copy link
Member

Choose a reason for hiding this comment

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

ack

@bwplotka
Copy link
Member

bwplotka commented Apr 3, 2020

See discussion here: #2365 and on slack https://cloud-native.slack.com/archives/CL25937SP/p1585903230146500

@bwplotka
Copy link
Member

bwplotka commented Apr 3, 2020

Alternative we agrred offline for: #2370 Let me know if that's make sense 🤗

@bwplotka
Copy link
Member

bwplotka commented Apr 6, 2020

Fix merged #2383

@bwplotka bwplotka closed this Apr 6, 2020
@bwplotka
Copy link
Member

bwplotka commented Apr 6, 2020

Thanks for help on this ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants