-
Notifications
You must be signed in to change notification settings - Fork 852
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
Add --X-trie-log subcommand #6303
Add --X-trie-log subcommand #6303
Conversation
|
20960ce
to
9f149a5
Compare
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/RocksDbUsageHelper.java
Outdated
Show resolved
Hide resolved
|
||
private static IdentityHashMap<byte[], byte[]> readTrieLogsFromFile() throws IOException { | ||
|
||
File file = new File(trieLogFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wold be better to use File.createTempFile so it's created in the temp directory and we won't have any write permissions issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the file to exist after we finish the run if for some reason we fail to reload the trielogs into the db
} | ||
|
||
private static void deleteTrieLogFile() { | ||
File file = new File(trieLogFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we create the file we could use the File.deleteOnExit so we don't need to worry about manually deleting this file
.ifPresent(trieLog -> trieLogsToRetain.put(hash.toArrayUnsafe(), trieLog)); | ||
}); | ||
out.println("Saving trielogs to retain in file..."); | ||
saveTrieLogsInFile(trieLogsToRetain); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how the fail-safe is being used. Expected to see that if we got an error clearing and inserting entries we ready from the file or allow specifying an existing backup file to restore from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment and instructions that will be logged for the user. Basically the idea here is, we retain the logs in a file in case something goes wrong.
In that case the user can re-run the subcommand and we will read the trielogs from the file since the column family might have been dropped already.
saveTrieLogsInFile(trieLogsToRetain); | ||
} else { | ||
// try to read the triLogs from the flatfile | ||
trieLogsToRetain = readTrieLogsFromFile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand how reading from the file helps in this case. We didn't find all the hashes but also haven't created the file at this point either unless we are relying running the command again to read the previous state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a failsafe in for a subsequent execution where something went terribly wrong during the truncation and restore-from-file process.
what would be nice, but not idiomatic, is if we had a few "are you sure?" or "do you want to?" prompts that could be bypassed by -y
similar to ubuntu apt commands
(key, value) -> { | ||
updater.getTrieLogStorageTransaction().put(key, value); | ||
}); | ||
updater.getTrieLogStorageTransaction().commit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails then do have a way of restoring the trielog from the backup?
Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
…tIdentifier.reset() Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
9f149a5
to
7401b59
Compare
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
…emory Signed-off-by: Gabriel Fukushima <[email protected]>
@@ -34,17 +34,17 @@ static void printUsageForColumnFamily( | |||
final RocksDB rocksdb, final ColumnFamilyHandle cfHandle, final PrintWriter out) | |||
throws RocksDBException, NumberFormatException { | |||
final String size = rocksdb.getProperty(cfHandle, "rocksdb.estimate-live-data-size"); | |||
final String numberOfKeys = rocksdb.getProperty(cfHandle, "rocksdb.estimate-num-keys"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes here seem fine but unrelated. Every time I look at this class name though I think it is a CLI Usage helper, like the output that gets printed when one asks for help or gets command usage wrong. completely unrelated, but if we are going to make changes in this pr ... :)
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Outdated
Show resolved
Hide resolved
private static IdentityHashMap<byte[], byte[]> readTrieLogsFromFile() throws IOException { | ||
|
||
File file = new File(trieLogFile); | ||
IdentityHashMap<byte[], byte[]> trieLogs = new IdentityHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trielogs can vary in size, and the retention count is user configurable. We could easily blow up the heap with bad settings. We might want to add some sanity check on the file size here, and perhaps also in trielog streaming/save to file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, users could eventually set the layersToRetain
to a high number and this could blow up. Is there real scenario where we should handle a high number having in mind that rolling back too many layer is not something feasible in bonsai (afaik)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trielogs can be the basis of a specific worldstate for debug_ and trace_ endpoints. But really if that were the case I imagine the node would be an archive node. I think streaming to and from disk would likely just be more bulletproof.
Not blocking feedback, just seems a robust mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this so we process it in batches now so it should be a safer mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think it wouldn't be too difficult to stream trielogs to and from the file directly to prevent OOM and heap issues.
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Show resolved
Hide resolved
LOG.info("Restoring trie logs retained from batch {}...", batchNumber); | ||
recreateTrieLogs(rootWorldStateStorage, batchNumber, batchFileNameBase); | ||
} catch (IOException | ClassNotFoundException e) { | ||
LOG.error("Error recreating trie logs from batch {}: {}", batchNumber, e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do at this point? Do we have a way to restart from an existing restore file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to change this to throw a new Runtime exception so we abort the process here, meaning if you re-run the command it's gonna detect the files exist and skip the save to file part but it will still reset the db and try to restore from file.
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
...reum/core/src/main/java/org/hyperledger/besu/ethereum/trie/bonsai/trielog/TrieLogPruner.java
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Outdated
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Outdated
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/subcommands/storage/TrieLogHelper.java
Show resolved
Hide resolved
Signed-off-by: Gabriel Fukushima <[email protected]>
…trie-log-subcommand-2
* mark deleted slot during clear storage step (hyperledger#6305) Signed-off-by: Karim Taam <[email protected]> Co-authored-by: garyschulte <[email protected]> * made directory structure of tests match source; fixed one typo (hyperledger#6337) Signed-off-by: Sally MacFarlane <[email protected]> * migrate controller tests to junit 5 (hyperledger#6338) Signed-off-by: Sally MacFarlane <[email protected]> * add new forkids for testnets, update forkid test to Junit5, no longer need named network specific trusted setups (hyperledger#6322) Signed-off-by: jflo <[email protected]> * Fix trielog shipping issue during self destruct (hyperledger#6340) * fix trielog shipping issue Signed-off-by: Karim Taam <[email protected]> * bump gradle properties version and adjust changelog to match release (hyperledger#6347) Signed-off-by: garyschulte <[email protected]> * finalized cancun spec (hyperledger#6351) * finalized cancun spec Signed-off-by: jflo <[email protected]> * finalized cancun spec Signed-off-by: jflo <[email protected]> --------- Signed-off-by: jflo <[email protected]> * Optimize RocksDB WAL file (hyperledger#6328) Signed-off-by: Fabio Di Fabio <[email protected]> * Make RPC reason settable, pass execution failure reason in RPC error message (hyperledger#6343) * Make RPC reason settable, pass execution failure reason in RPC error message Signed-off-by: Matthew Whitehead <[email protected]> * Update unit tests Signed-off-by: Matthew Whitehead <[email protected]> * Update tests Signed-off-by: Matthew Whitehead <[email protected]> * Update change log Signed-off-by: Matthew Whitehead <[email protected]> * Update integration tests Signed-off-by: Matthew Whitehead <[email protected]> --------- Signed-off-by: Matthew Whitehead <[email protected]> * TestWatcher junit5 (hyperledger#6339) * TestWatcher junit5 * add test class and method name to context * moved the testwatcher junit5 function to a new junit5 superclass * one qbft test to junit5 superclass Signed-off-by: Sally MacFarlane <[email protected]> --------- Signed-off-by: Sally MacFarlane <[email protected]> Co-authored-by: Stefan Pingel <[email protected]> * Migrate BFT tests to junit 5 (hyperledger#6350) * bft tests to junit 5 * base class for pki extend AcceptanceTestBaseJunit5 * try/catch in case of empty optionals * fixed parameterization method Signed-off-by: Sally MacFarlane <[email protected]> --------- Signed-off-by: Sally MacFarlane <[email protected]> * fixing on selfdestruct (hyperledger#6359) Signed-off-by: Karim Taam <[email protected]> * migrate clique tests fully to junit5 (hyperledger#6362) * migrate clique tests fully to junit5 Signed-off-by: Sally MacFarlane <[email protected]> --------- Signed-off-by: Sally MacFarlane <[email protected]> * fixed link to logging docs (hyperledger#6366) Signed-off-by: Sally MacFarlane <[email protected]> * Move logging to RunnerBuilder (hyperledger#6367) Signed-off-by: Gabriel-Trintinalia <[email protected]> * Use synchronized call to access the chain head block in `eth_estimateGas` (hyperledger#6345) * Use synchronized call to access the chain head block in estimateGas() Signed-off-by: Matthew Whitehead <[email protected]> * Add error log entries when throwing internal error from estimateGas() Signed-off-by: Matthew Whitehead <[email protected]> * Update unit tests Signed-off-by: Matthew Whitehead <[email protected]> * Update changelog Signed-off-by: Matthew Whitehead <[email protected]> --------- Signed-off-by: Matthew Whitehead <[email protected]> * Add --X-trie-log subcommand (hyperledger#6303) * Add x-trie-log subcommand for one-off trie log backlog prune Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> --------- Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> Co-authored-by: Simon Dudley <[email protected]> * fix typos (hyperledger#6368) Signed-off-by: vuittont60 <[email protected]> * Added alias --sync-min-peers for --fast-sync-min-peers (hyperledger#6372) * sync-min-peers as an alias * added unit tests Signed-off-by: Sally MacFarlane <[email protected]> --------- Signed-off-by: Sally MacFarlane <[email protected]> * Fix: Fallback to getName when canonicalName is null in BlockHeaderValidator DEBUG log (hyperledger#6332) * fallback to simple name when canonical name is null * use getName instead of getSimpleName to include the package name Signed-off-by: Manoj P R <[email protected]> --------- Signed-off-by: Manoj P R <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]> * fix: use UID 1000 for besu user (hyperledger#6358) (hyperledger#6360) The openjdk-latest Docker image is using UID 1001 for besu, because its base image ubuntu:23.10 now contains a default "ubuntu" user with UID 1000. (This UID change causes the besu user with UID 1001 to not have access to files created for past versions with UID 1000.) We now remove the default ubuntu user and explicitly use UID 1000 when creating the besu user. Signed-off-by: Hal Blackburn <[email protected]> * Ignore generated files when running the spdx license check (hyperledger#6379) Signed-off-by: Meredith Baxter <[email protected]> * full sync - don't fail startup if sync-min-peers specified (hyperledger#6373) Signed-off-by: Sally MacFarlane <[email protected]> * Copy also computed fields, when doing a Transaction detached copy (hyperledger#6329) Signed-off-by: Fabio Di Fabio <[email protected]> * Disable txpool when not in sync (hyperledger#6302) Signed-off-by: Fabio Di Fabio <[email protected]> * Bump to nex release snapshot 24.1.1 (hyperledger#6383) * release next snapshot 24.1.1 Signed-off-by: garyschulte <[email protected]> * move recent changelog items to 24.1.1-SNAPSHOT Signed-off-by: garyschulte <[email protected]> --------- Signed-off-by: garyschulte <[email protected]> * Correct Tangerine Whistle definition in Fluent EVM APIs. (hyperledger#6382) The fluent API incorrectly added the code size limit in Tangerine Whistle instead of first adding it in Spurious Dragon. Signed-off-by: Danno Ferrin <[email protected]> * [MINOR] Fix pki tests condition check on mac (hyperledger#6387) Signed-off-by: Gabriel-Trintinalia <[email protected]> * Upgrade dependencies (hyperledger#6377) * Bump com.github.oshi:oshi-core to 6.4.10 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.github.tomakehurst to org.wiremock 3.3.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.google.auto.service:auto-service to 1.1.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.google.dagger group to 2.50 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.graphql-java:graphql-java to 21.3 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.splunk.logging:splunk-library-javalogging to 1.11.8 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump com.squareup.okhttp3:okhttp to 4.12.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump commons-io:commons-io to 2.15.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump dnsjava:dnsjava to 3.5.3 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump info.picocli group to 4.7.5 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump io.grpc group to 1.60.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump io.kubernetes:client-java to 18.0.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump io.netty group to 4.1.104.Final Signed-off-by: Fabio Di Fabio <[email protected]> * Bump net.java.dev.jna:jna to 5.14.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.apache.commons:commons-compress to 1.25.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.apache.commons:commons-lang3 to 3.14.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.apache.commons:commons-text to 1.11.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.apache.logging.log4j group to 2.22.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Redorder io.tmio group Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.assertj:assertj-core to 3.25.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.bouncycastle group to 1.77 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.fusesource.jansi:jansi to 2.4.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.immutables group 2.10.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.java-websocket:Java-WebSocket to 1.5.5 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.jetbrains.kotlin:kotlin-stdlib to 1.9.22 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.junit.jupiter group to 5.10.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.jupnp group to 2.7.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.rocksdb:rocksdbjni to 8.9.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.slf4j group to 2.0.10 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.springframework.security:spring-security-crypto to 6.2.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.testcontainers:testcontainers to 1.19.3 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.web3j group to 4.10.3 Signed-off-by: Fabio Di Fabio <[email protected]> * Bump org.xerial.snappy:snappy-java to 1.1.10.5 Signed-off-by: Fabio Di Fabio <[email protected]> * Regenerate gradle verification metadata Signed-off-by: Fabio Di Fabio <[email protected]> * Update commons-codec:commons-codec to 1.16.0 Signed-off-by: Fabio Di Fabio <[email protected]> * Update org.junit.vintage:junit-vintage-engine to 5.10.1 Signed-off-by: Fabio Di Fabio <[email protected]> * Update CHANGELOG Signed-off-by: Fabio Di Fabio <[email protected]> --------- Signed-off-by: Fabio Di Fabio <[email protected]> * add a fallback for docker detection on Mac (hyperledger#6356) Signed-off-by: garyschulte <[email protected]> * Fix test flackyness of acceptanceTestsPermissioning (hyperledger#6384) Signed-off-by: Fabio Di Fabio <[email protected]> Signed-off-by: Sally MacFarlane <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]> * Upgrade `com.fasterxml.jackson` dependencies (hyperledger#6378) Signed-off-by: Fabio Di Fabio <[email protected]> * Use mining beneficiary from protocol spec in TraceServiceImpl (hyperledger#6390) * use mining beneficiary from protocol spec Signed-off-by: Daniel Lehrner <[email protected]> --------- Signed-off-by: Daniel Lehrner <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]> * Update verification metadata and allowed licenses for Linea-Besu --------- Signed-off-by: Karim Taam <[email protected]> Signed-off-by: Sally MacFarlane <[email protected]> Signed-off-by: jflo <[email protected]> Signed-off-by: garyschulte <[email protected]> Signed-off-by: Fabio Di Fabio <[email protected]> Signed-off-by: Matthew Whitehead <[email protected]> Signed-off-by: Gabriel-Trintinalia <[email protected]> Signed-off-by: Simon Dudley <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> Signed-off-by: vuittont60 <[email protected]> Signed-off-by: Manoj P R <[email protected]> Signed-off-by: Hal Blackburn <[email protected]> Signed-off-by: Meredith Baxter <[email protected]> Signed-off-by: Danno Ferrin <[email protected]> Signed-off-by: Daniel Lehrner <[email protected]> Co-authored-by: Karim TAAM <[email protected]> Co-authored-by: garyschulte <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]> Co-authored-by: Justin Florentine <[email protected]> Co-authored-by: Fabio Di Fabio <[email protected]> Co-authored-by: Matt Whitehead <[email protected]> Co-authored-by: Stefan Pingel <[email protected]> Co-authored-by: Gabriel-Trintinalia <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]> Co-authored-by: Simon Dudley <[email protected]> Co-authored-by: vuittont60 <[email protected]> Co-authored-by: Manoj P R <[email protected]> Co-authored-by: Hal Blackburn <[email protected]> Co-authored-by: mbaxter <[email protected]> Co-authored-by: Danno Ferrin <[email protected]> Co-authored-by: daniellehrner <[email protected]>
PR description
This is a more direct approach to the previous attempt in #6188.
Tests showed that the previous approach was taking too long (3 hours) to prune a 3.5 months old (keeping the lastest 512 only).
The approach this PR takes is faster since it resets the column family and only retain the necessary trie logs (saved in files in batches of 20_000 trie logs).
Drawback of this approach is that we do store the trie logs in disk (in files) before dropping the column. Realistic speaking there's little to no reason to actually keep many more trie logs than the default(512).
In any case, I've made some adjustments to the code to allow this to go as far as a user might like.
Ran tests on nodes retaining 512 / 20_000 / 250_000 / 1M trie logs, all instances had approx 1.8M trie logs in the db when node was stopped for prunning.
Below some of the logs condensed
Real use case scenario - 512 trie logs retained (1 second)
a more extreme case where for some reason someone has been running an archieve and decides to keep only the latest 1M trie logs - 1M trie logs retained (approx 2 hours)
Fixed Issue(s)
Part of #5390