Skip to content

MINOR:Remove explicit flushes from RocksDBStores#21871

Open
eduwercamacaro wants to merge 1 commit intoapache:trunkfrom
eduwercamacaro:remove-flush-for-rocksdb-stores
Open

MINOR:Remove explicit flushes from RocksDBStores#21871
eduwercamacaro wants to merge 1 commit intoapache:trunkfrom
eduwercamacaro:remove-flush-for-rocksdb-stores

Conversation

@eduwercamacaro
Copy link
Copy Markdown
Contributor

@eduwercamacaro eduwercamacaro commented Mar 25, 2026

RocksDBStores now manage their own offsets by storing them in a
offsets ColumnFamily. Also, these stores use a RocksDB atomic flush to
guarantee consistency between the default and offsets ColumnFamilies. As
described in KIP-1035, this new behavior enables RocksDB stores to avoid
explicitly flushing the memtables to the SST files.

This is a follow-up PR for #21578 we
can now implement this because KAFKA-19712 is solved.

Reviewers: NIck Telford nick.telford@gmail.com, Bill Bejeck
bbejeck@apache.org

…ts ColumnFamily. Also, these stores use a RocksDB atomic flush to guarantee consistency between the default and offsets ColumnFamilies. As described in KIP-1035, this new behavior enables RocksDB stores to avoid explicitly flushing the memtables to the SST files
@github-actions github-actions bot added triage PRs from the community streams small Small PRs labels Mar 30, 2026
@bbejeck bbejeck changed the title Remove explicit flushes from RocksDBStores MINOR:Remove explicit flushes from RocksDBStores Mar 30, 2026
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eduwercamacaro - LGTM with one minor comment


accessor.commit(dbAccessor, offsets);

verify(dbAccessor).flush(oldCF, newCF, offsetsCF);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add another assertion here? Otherwise it's an empty test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

small Small PRs streams triage PRs from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants