Resolve Parquet shard count via bucket index to optimize storage calls

[SungJin1212]

What this PR does:
This PR updates the Parquet shard resolution logic to utilize the bucket index, reducing the number of object storage calls.

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
• docs/configuration/v1-guarantees.md updated if this PR introduces experimental flags

[yeya24]

This should be cached with some TTL instead? Or we would rather have a separate goroutine to sync bucket index periodically rather than resolving it at query time.

[yeya24]

If we use bucketindex.Loader it has built-in caching

[SungJin1212]

@yeya24
I adapted bucketindex.Loader to the parquet store gateway.

• In InitialSync, I start the indexLoader.
• Its metrics are tagged with component="store-gateway" so they don't collide with the querier's loader in single-binary mode.

[yeya24]

I wonder how this bucket index integration works with the Store gateway hybrid mode mentioned in #7140.

Do we imagine the hybrid mode to be as part of TSDB bucket store, or Parquet bucket store or a new federated store.

[SungJin1212]

@yeya24
I haven't thought about hybrid mode deeply yet.. but my preference would be a new federated store that holds both the TSDB and parquet bucket stores and splits blocks using the bucket index.

[SungJin1212]

@yeya24
How about extending the Parquet bucket store into hybrid mode, since the current Parquet SG has a query hole?

[yeya24]

Should we try to cache the shard counts result? Because every time we get the whole bucket index and iterate all its blocks to populate shard map. Even though the bucket index is cached, re-populating the map seems wasted. Especially if we only try to query few block IDs among the bucket index

[SungJin1212]

Agree, I addressed this by memoizing the shard count map instead of rebuilding it on every request.

[yeya24]

Overall I think this is fine... But for most of the usecases we only have 1 shard for the parquet file.

[friedrichg]

LGTM

[friedrichg]

Imagine having 2 shards in a block, but the bucket index doesn't have it yet. That means we get 1 shard only.

I think, for cases where query_store_after is configured as zero, the default, this would cause partial queries in bucket index blocks that don't have shards in them. But then again who is using query_store_after with zero?

I think we should definitely change the default for query_store_after to be bigger than 0 and discourage very low query store after in general. Not good performance.

Then again, what about backfilling up historical blocks to object storage. Maybe we should fall back to the following instead of using 1

func (p *parquetBucketStore) shardCountFromConverterMark(ctx context.Context, blockID string) (int, error) {
	uid, err := ulid.Parse(blockID)
	if err != nil {
		return 0, errors.Wrapf(err, "failed to parse block ID %s", blockID)
	}
	marker, err := cortex_parquet.ReadConverterMark(ctx, uid, p.bucket, p.logger)
	if err != nil {
		return 0, errors.Wrapf(err, "failed to read converter mark for block %s", blockID)
	}
	return marker.Shards, nil
}

We can definitely do this in a follow up PR

[SungJin1212]

Then again, what about backfilling up historical blocks to object storage. Maybe we should fall back to the following instead of using 1

Agree, we need to read the block's converter mark directly on a map miss.

I think we should definitely change the default for query_store_after to be bigger than 0 and discourage very low query store after in general. Not good performance.

Agree on discouraging very low values for performance, but query_store_after is coupled with the query_ingesters_within. So bumping the query_store_after default would also require bumping query_ingesters_within, which is a broader change affecting all users. I'd keep that as a separate discussion.