kafka replay speed: don't trim fetchWants

[dimitarvdimitrov]

What this PR does

I realized that trimming fetchWants can end up discarding offsets in extreme circumstances.

How it works

If the fetchWant is so big that its size would exceed 2GiB, then we trim it. We trim it by reducing the end offset. The idea is that the next fetchWant will pick up from where this one left off.

How it can break

We trim the fetchWant in UpdateBytesPerRecord too. UpdateBytesPerRecord can be invoked in concurrentFEtchers.run after the fetchWant is dispatched. In that case the next fetchWant would have already been calculated. And we would end up with a gap.

Did it break?

It's hard to tell, but it's very unlikely. To reach 2GiB we would have needed to have the estimation for bytes per record be 2 MiB (with 1000 records per fetch). While these large records are possible, they should be rare and our rolling average estimation for records size shouldn't reach it.

Why we can do without trimIfTooBig()?

MaxBytes() already caps the value to MaxInt32, so we're compliant with the Kafka Protocol.

mimir/pkg/storage/ingest/fetcher.go

Lines 97 to 102 in 175bef4

	fetchBytes := w.expectedBytes()
	if fetchBytes > math.MaxInt32 {
	// This shouldn't happen because w should have been trimmed before sending the request.
	// But we definitely don't want to request negative bytes by casting to int32, so add this safeguard.
	return math.MaxInt32
	}

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[pracucci]

I see the problem. Good job spotting it!

Do you think we can unit test fetchWatch.MaxBytes() and UpdateBytesPerRecord() for this specific case the fetch would be greater than int32 bytes? I know we removed the trim code, but could help us not reintroduce something similar in the future.

[dimitarvdimitrov]

Do you think we can unit test fetchWatch.MaxBytes() and UpdateBytesPerRecord() for this specific case the fetch would be greater than int32 bytes? I know we removed the trim code, but could help us not reintroduce something similar in the future.

added some tests here and had to adjust MaxBytes slightly because it could overflow sometimes dcc1397