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

feat(estimator): compact RocksDB in testbed setup #7502

Merged
merged 7 commits into from
Aug 30, 2022

Conversation

jakmeier
Copy link
Contributor

Run compaction on the DB after inserting all records from the state
dump. This brings the layout of SST files to a more stable and optimal
layout. See #4771 for more details.

This is an estimator only change.

Run compaction on the DB after inserting all records from the state
dump. This brings the layout of SST files to a more stable and optimal
layout. See near#4771 for more details.
@jakmeier jakmeier requested a review from mina86 August 30, 2022 10:09
@jakmeier jakmeier requested a review from a team as a code owner August 30, 2022 10:09
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM!

But let's document the knowledge of "why" we are running compaction, and what breaks if we don't. Most obvious way is on the store.compact() call, but, if we have some better overview of the whole process somewhere (readme? top comment in lib.rs), that would be even a better place.

@@ -247,6 +247,14 @@ impl Database for RocksDB {
self.db.write(batch).map_err(into_other)
}

fn compact(&self) -> io::Result<()> {
let none = Option::<&[u8]>::None;
Copy link
Contributor

Choose a reason for hiding this comment

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

And that's why I have a distace for AsRef-polymorphism for nicer APIs :)

- short comment why `compact()` is called
- document the issue in `runtime/runtime-params-estimator/src/lib.rs`
- remove default implementation of `Database::copmact`
@jakmeier
Copy link
Contributor Author

LGTM!

But let's document the knowledge of "why" we are running compaction, and what breaks if we don't. Most obvious way is on the store.compact() call, but, if we have some better overview of the whole process somewhere (readme? top comment in lib.rs), that would be even a better place.

I added some documentation in runtime/runtime-params-estimator/src/lib.rs and a short comment where it is actually called. Do you think that is sufficiently detailed and sufficiently discoverable?

@jakmeier jakmeier requested a review from matklad August 30, 2022 10:53
Comment on lines +140 to +143
/// Compact database representation.
///
/// If the database supports it a form of compaction, calling this function
/// is blocking until compaction finishes. Otherwise, this is a no-op.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Compact database representation.
///
/// If the database supports it a form of compaction, calling this function
/// is blocking until compaction finishes. Otherwise, this is a no-op.
/// Compacts database representation.
///
/// If the database supports compaction, calling this function blocks
/// until compaction finishes. Otherwise, this is a no-op.

@near-bulldozer near-bulldozer bot merged commit 23e0fec into near:master Aug 30, 2022
@jakmeier jakmeier deleted the rocksdb-estimator-compact branch August 30, 2022 13:28
nikurt pushed a commit that referenced this pull request Nov 9, 2022
Run compaction on the DB after inserting all records from the state
dump. This brings the layout of SST files to a more stable and optimal
layout. See #4771 for more details.

This is an estimator only change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants