Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Compaction index uses hardcoded 512kB memory limit #4645

Closed
jcsp opened this issue May 10, 2022 · 2 comments · Fixed by #5722
Closed

storage: Compaction index uses hardcoded 512kB memory limit #4645

jcsp opened this issue May 10, 2022 · 2 comments · Fixed by #5722
Assignees
Labels
area/storage kind/enhance New feature or request

Comments

@jcsp
Copy link
Contributor

jcsp commented May 10, 2022

For topics with compaction enabled, the compaction writer's inner spill_key_index may use up to 512kB of memory for its unflushed data. This is initialized as

                make_file_backed_compacted_index(
                  path.string(),
                  writer,
                  iopc,
                  segment_appender::write_behind_memory / 2));

(segment_appender::write_behind memory is 1 meg).

512kB is too big when we have huge partition counts.

It is ameliorated somewhat by the relatively smaller number of events per partition when the partition count is huge, but for tiny messages we can still end up using the whole 512kB. Consider a writer doing tiny events with a 64 byte key and 64 byte value, with 100k partitions and 2TB of storage, where they keys do not repeat often. That's 156250 events per partition before they run out of disk space, multiply by the 64 byte key size.

So: to avoid using 50GB of memory for compaction indices on a node with 100k partitions, we need to do something smarter. This is a bit similar to #4600 in that the solution can be either an explicit config, or ideally something more dynamic. We could consider having a total per-shard memory limit for compaction index space, rather than a per-partition space limit.

@jcsp jcsp added kind/enhance New feature or request area/storage labels May 10, 2022
@emaxerrno
Copy link
Contributor

@jcsp the per shard shared mem. Makes a lot of sense. Similar to what Noah did for the chunk appender. What if we remove the memory argument altogether and that would
Just come from a pool like you
Mentioned - good ideas !

@jcsp jcsp self-assigned this May 26, 2022
@jcsp
Copy link
Contributor Author

jcsp commented Jul 26, 2022

This is broadly the same as #5389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage kind/enhance New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants