objstore: Added WithExpectedErrs which allows to control instrumentation (e.g not increment failures for expected not found). #2370
Conversation
bwplotka
force-pushed
the
error-not-foun
branch
3 times, most recently
from
f777523
to
2a94dc3
Compare
…strumentation (e.g not increment failures for expected not found). This allows to not wake up oncall in the middle of night, becuase of expeced, properly handled case (: Also: Has to move inmem to objstore for testing. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
There was a problem hiding this comment.
Seems like you've renamed some more things than needed. There were even more but I stopped adding comments to avoid spam. But overall I like this 👍 it addresses both of the concerns that we shouldn't blindly ignore 404 and the other concern by the Cortex folks that we might start doing too many operations if we would start explicitly checking the existence of those objects. Good work! This is definitely v0.12.0 material :P
| @@ -64,7 +64,7 @@ func (conf *Config) validate() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // NewBucket returns a new Bucket using the provided Azure config. | |||
| // NewInMemBucket returns a new Bucket using the provided Azure config. | |||
There was a problem hiding this comment.
Seems like an unrelated change?
| @@ -40,9 +40,9 @@ type BucketConfig struct { | |||
| Config interface{} `yaml:"config"` | |||
| } | |||
|
|
|||
| // NewBucket initializes and returns new object storage clients. | |||
| // NewInMemBucket initializes and returns new object storage clients. | |||
There was a problem hiding this comment.
Seems like an unrelated change?
| @@ -43,7 +43,7 @@ func NewBucketFromConfig(conf []byte) (*Bucket, error) { | |||
| return NewBucket(c.Directory) | |||
| } | |||
|
|
|||
| // NewBucket returns a new filesystem.Bucket. | |||
| // NewInMemBucket returns a new filesystem.Bucket. | |||
| @@ -41,7 +41,7 @@ type Bucket struct { | |||
| closer io.Closer | |||
| } | |||
|
|
|||
| // NewBucket returns a new Bucket against the given bucket handle. | |||
| // NewInMemBucket returns a new Bucket against the given bucket handle. | |||
| @@ -150,7 +150,7 @@ func (b *Bucket) ObjectSize(ctx context.Context, name string) (uint64, error) { | |||
| return 0, errors.New("content-length header not found") | |||
| } | |||
|
|
|||
| // NewBucket returns a new Bucket using the provided oss config values. | |||
| // NewInMemBucket returns a new Bucket using the provided oss config values. | |||
There was a problem hiding this comment.
Good job @bwplotka and thanks for taking care of this! LGTM (modulo @GiedriusS comments).
| @@ -236,7 +236,7 @@ func (f *BaseFetcher) loadMeta(ctx context.Context, id ulid.ULID) (*metadata.Met | |||
| } | |||
| } | |||
|
|
|||
| r, err := f.bkt.Get(ctx, metaFile) | |||
| r, err := f.bkt.ReaderWithExpectedErrs(f.bkt.IsObjNotFoundErr).Get(ctx, metaFile) | |||
There was a problem hiding this comment.
Is this expected to cover the case of a partially uploaded block (while still uploading)?
|
Exactly this + Giedrius addressed comments was merged in release 0.12 and will be ported to master soon: c05daee |
This 3rd alternative to below PRs that was discussed offline: https://cloud-native.slack.com/archives/CL25937SP/p1585903230146500
Closes #2365 and #2369
This allows to not wake up on-call in the middle of the night, because of the expected, properly handled case (:
Thanks @pracucci and @pstibrany for starting this work and discussion. We were aware of this for like 12 months, but only with your discussions, we clarified what is expected here 🤗 Your input was super valuable.
Changes
Added new interface:
Also: Has to move inmem to objstore for testing.
Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com