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.

Sign up for GitHub

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

Jump to bottom

Report estimated disk space freed up by x-trie-log prune subcommand #6483

Merged
merged 15 commits into from
Feb 1, 2024
Merged

Report estimated disk space freed up by x-trie-log prune subcommand #6483

merged 15 commits into from
Feb 1, 2024

Conversation

siladu
Copy link
Contributor

@siladu siladu commented Jan 29, 2024 •
edited
Loading

Prune ran successfully. We estimate you freed up 8 GiB! 🚀

Also add --Xbonsai-trie-log-pruning-enabled alias back in since users have started using this already

siladu added 3 commits January 29, 2024 15:42
@siladu
WIP
f9f762d 
Signed-off-by: Simon Dudley <[email protected]>
@siladu
Refactor
47e9f43 
Signed-off-by: Simon Dudley <[email protected]>
@siladu
Refactor
7a55c97 
Signed-off-by: Simon Dudley <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2024 •
edited by siladu
Loading

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@siladu siladu added the TeamGroot GH issues worked on by Groot Team label Jan 29, 2024
@siladu siladu self-assigned this Jan 29, 2024
jframe
jframe reviewed Jan 30, 2024
View reviewed changes
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/RocksDbUsageHelper.java Outdated
@@ -30,6 +35,37 @@
public class RocksDbUsageHelper {
private static final Logger LOG = LoggerFactory.getLogger(RocksDbUsageHelper.class);

static void forEachColumnFamily(
final String dbPath, final BiFunction<RocksDB, ColumnFamilyHandle, Void> task) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you changed the type from BiFunction<RocksDB, ColumnFamilyHandle, Void> to BiConsumer<RocksDB, ColumnFamilyHandle> then you could get rid of the return null in TrieLogSubCommand and RocksDbUsageHelper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, thanks!

besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/RocksDbUsageHelper.java
@@ -30,6 +35,37 @@
public class RocksDbUsageHelper {
private static final Logger LOG = LoggerFactory.getLogger(RocksDbUsageHelper.class);

static void forEachColumnFamily(
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't really belong in the RocksDbUsageHelper now that it's more generic. Perhaps move it out into a RocksDbHelper class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. Have renamed class now it's not just about Usage

besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogSubCommand.java Outdated
if (Arrays.equals(cfHandle.getName(), TRIE_LOG_STORAGE.getId())) {
estimatedSaving.set(
Long.parseLong(
rocksdb.getProperty(cfHandle, "rocksdb.estimate-live-data-size")));
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the savings. It's only the estimated savings if a small number of trielogs is kept. Think could be misleading for some people.

If we are going to print the estimated savings then we should compare before and after data size of the trie log column family.

Copy link
Contributor Author

@siladu siladu Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't really want to slow the subcommand down even further for all users just for an edge case. Think this is an acceptable trade-off because it's just an estimate anyway. Would you block the PR on this?

Copy link
Contributor Author

@siladu siladu Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also I don't think reevaluating the size after will work within the same subcommand without manually triggering the compaction. Even if it did, that second measurement too would be an estimate, potentially with a error range that is greater than the actual new size of the column family.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spiking this out here: #6492

Results from local test...

Trie logs size before: 578 KiB
Trie logs size after: 0 B
Prune ran successfully. We estimate you freed up 578 KiB! 🚀

Copy link
Contributor Author

@siladu siladu Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Results from a (not very old and already pruned) mainnet node:

2024-01-30 05:51:43.982+00:00 | main | INFO  | RocksDBKeyValueStorageFactory | Existing database detected at /data/besu. Version 2. Compacting database...
2024-01-30 05:51:45.286+00:00 | main | INFO  | TrieLogSubCommand | Calculating trie logs size before...
2024-01-30 05:52:38.295+00:00 | main | INFO  | TrieLogSubCommand | Trie logs size before: 30 MiB
...
2024-01-30 05:52:38.819+00:00 | main | INFO  | TrieLogSubCommand | Finished pruning - recalculating size...
2024-01-30 05:52:39.271+00:00 | main | INFO  | TrieLogSubCommand | Trie logs size after: 0 B
Prune ran successfully. We estimate you freed up 30 MiB! 🚀

Compaction was fast, "before" estimate nearly took 1 minute, "after" estimate was fast but didn't add any data.

Will try this on an older node next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested on a larger node...

2024-01-31 03:27:06.481+00:00 | main | INFO  | TrieLogSubCommand | Finished pruning - recalculating size...
2024-01-31 03:27:07.034+00:00 | main | INFO  | TrieLogSubCommand | Trie logs size after: 0 B
Prune ran successfully. We estimate you freed up 8 GiB! 🚀

So yeh, I think we'll need to compact to see any "size after"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better results when the retained trie logs value is higher actually...

$ sudo /opt/besu/current/bin/besu --data-storage-format=BONSAI --data-path=/data/besu --bonsai-historical-block-limit=20000 storage x-trie-log prune

...
2024-01-31 04:32:04.021+00:00 | main | INFO  | RocksDBKeyValueStorageFactory | Existing database detected at /data/besu. Version 2. Compacting database...
...
2024-01-31 04:32:05.229+00:00 | main | INFO  | TrieLogSubCommand | Calculating trie logs size before...
2024-01-31 04:32:05.683+00:00 | main | INFO  | TrieLogSubCommand | Trie logs size before: 10 GiB
2024-01-31 04:32:17.623+00:00 | main | INFO  | TrieLogHelper | Saving trie logs to retain in file /data/besu/database/trieLogsToRetain-1 (batch 1)...
2024-01-31 04:32:17.625+00:00 | main | INFO  | TrieLogHelper | Obtaining trielogs from db, this may take a few minutes...
2024-01-31 04:32:39.579+00:00 | main | INFO  | TrieLogHelper | Clear trie logs...
2024-01-31 04:32:43.786+00:00 | main | INFO  | TrieLogHelper | Restoring trie logs retained from batch 1...
...
2024-01-31 04:33:46.212+00:00 | main | INFO  | TrieLogHelper | Deleting files...
2024-01-31 04:33:46.294+00:00 | main | INFO  | TrieLogSubCommand | Finished pruning - recalculating size...
2024-01-31 04:33:57.324+00:00 | main | INFO  | TrieLogSubCommand | Trie logs size after: 546 MiB
Prune ran successfully. We estimate you freed up 9 GiB! 🚀

Copy link
Contributor Author

@siladu siladu Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Given the results and the fact not much extra time is added on the second size calculation, I think this is worth including this PR, thanks for the feedback 😄

@siladu siladu mentioned this pull request Jan 30, 2024
Closed
@siladu siladu merged commit 8f2ee84 into hyperledger:main Feb 1, 2024
28 checks passed
@siladu siladu deleted the trielog-disk-freed-report branch February 1, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfukushima gfukushima gfukushima approved these changes

@jframe jframe jframe approved these changes

Assignees

@siladu siladu

Labels
TeamGroot GH issues worked on by Groot Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@siladu @jframe @gfukushima