ConsistencyDelayMetaFilter should filter on the block upload time and not the block creation time

[pracucci]

The ConsistencyDelayMetaFilter filter currently filters out blocks based on the ULID time, which is the time when the block has been created:

thanos/pkg/block/fetcher.go

Line 727 in cfced59

if ulid.Now()-id.Time() < uint64(f.consistencyDelay/time.Millisecond) &&

This means that if that actual consistency delay is the configured delay minus the upload time. If the delay is X but the block upload time is > X, the consistency delay is effectively ignored.

To correct approach would be checking the consistency delay against the "block upload completed timestamp" and, to do so, checking the meta.json creation time could be fine. However, object stores stores the "updated time" and not the "creation time", so I'm wondering how safe would be using it. Thoughts?

[bwplotka]

There was already issue for that...

[bwplotka]

I can't find this ticket ;p So let's keep this.

Definitely todo. We did not have just yet modified Time API in objstore that's the reason why. Modified will be just enough I think, even though creation would be better here. The difficulty it so add working implementation of mod time to all implementations now.

We started some work in some old PRs. I think we also thought getting the Size, ModTime in one method Attr which is common pattern.

[pracucci]

We started some work in some old PRs. I think we also thought getting the Size, ModTime in one method Attr which is common pattern.

The problem of doing it is that we'll double the API calls for this specific use case, because most of the object stores allow to get it along with the GET object API call (as response HTTP headers).

How would you feel about changing the BucketReader.Get() signature from:
Get(ctx context.Context, name string) (io.ReadCloser, error)

To:
Get(ctx context.Context, name string) (io.ReadCloser, *ObjectInfo, error)

Where ObjectInfo is:

type ObjectInfo struct {
    Size uint64
    LastModified time.Time
}

With the contract that if such information are unavailable return returned ObjectInfo is nil (basically a best effort).

[pracucci]

On the other side, adding a ObjectInfo() function would reduce the surface change and it's also true that could be a piece of information that could be cached by #2579.

[pstibrany]

It seems weird for it to be part of Get, even when we try to optimize for number of requests (which is not a problem for all object stores). There may be legitimate use-cases to need that data without needing content of object. I think dedicated method would be better way to design this.

[pracucci]

After a brief discussion on Slack, looks there's consensus for adding the Attributes(ctx, name) (ObjectInfo, error) function to BucketReader, including Size and LastModified for now.

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[bwplotka]

Still valid and actually possible thanks to recent changes. Some description why it's important you can see here: #2470