Fix compaction index size overflow #7687
Conversation
ztlpn
force-pushed
the
fix-7647-compaction-index-overflow
branch
2 times, most recently
from
e2af2cb
to
fd4bcc9
Compare
If the index is of different version than the current one, request a rebuild. It is easier than dealing with compatibility issues.
ztlpn
force-pushed
the
fix-7647-compaction-index-overflow
branch
from
fd4bcc9
to
3ad52b8
Compare
| ss::temporary_buffer<char> tmp = co_await in.read_exactly( | ||
| compacted_index::footer_size); |
There was a problem hiding this comment.
nit: can we use read_iobuf_exactly ?
|
|
||
| if (tmp.size() != compacted_index::footer_size) { | ||
| if (buf.size_bytes() != compacted_index::footer_size) { | ||
| throw std::runtime_error(fmt::format( | ||
| "could not read enough bytes to parse " |
There was a problem hiding this comment.
Is this the case we hit when loading old cluster's compaction index? If so, maybe we should make the message a bit clear to indicate that this isn't totally unexpected.
There was a problem hiding this comment.
It is kind of a catch-all, but adding a note about a likely cause is a good idea.
|
@ztlpn thanks so much for this. i did a quick git blame on it ... and the original thinking was that segments would be 4G max, at some point along the way i forgot to add the assert. At the time of writing this, we were actually capping segments at 2G, so I figured why not 2X. it's all more bits now :) thanks for the fix. |
|
Just to check we're thinking the same thing -- the regeneration of v1 indices is expected to happen on-demand during compaction, right? So we won't be doing some kind of mass rewrite when we start up... |
As by default max compacted segment size is 5 GiB, compacted index size for large segments can oveflow 32-bit ints. This commit substitutes size and the number of keys in the footer for 64-bit ints. Old 32-bit fields are left for backwards compatibility with the code that doesn't check version number. Fixes redpanda-data#7647
ztlpn
force-pushed
the
fix-7647-compaction-index-overflow
branch
from
9700e4d
to
3933d29
Compare
Good point, I did a quick code audit, looks like we only read compaction index during actual compaction. |
|
@ztlpn before we merge a backport + release this to the field, perhaps it is worth adding a ducktape test in a followup PR that ensures this works end to end across the upgrade? (including the rewrites that we expect to happen via exceptions) |
|
sure, will do |
Clamp max compacted segment size to 1.5GiB to avoid compaction index size overflow. Fixes redpanda-data#7647 This is a workaround for older versions, proper fix is redpanda-data#7687
Clamp max compacted segment size to 1.5GiB to avoid compaction index size overflow. Fixes redpanda-data#7647 This is a workaround for older versions, proper fix is redpanda-data#7687
As by default max compacted segment size is 5 GiB, compacted index size for large segments can oveflow 32-bit ints. This PR substitutes size and the number of keys in the footer for 64-bit ints. Old 32-bit fields are left for backwards compatibility with the code that doesn't check version number.
Fixes #7647
Additionally a more stringent footer version check is introduced that requires version to be equal to the current one (it is better to rebuild even if the version is greater as the index is probably incompatible anyway).
Backports Required
Release Notes
Bug Fixes