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

Snapsync persist state #4381

Merged
merged 15 commits into from
Nov 1, 2022
Merged

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Sep 12, 2022

PR description

This PR avoids starting the download of the world state from scratch when restarting Besu

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…re/snapsync-persist

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt marked this pull request as ready for review September 19, 2022 14:45
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

intuitively I think that keeping the pending and inconsistent ranges in files in the filesystem rather than in rocksdb would be more flexible than adding column families. It is clearly a lot more data, but it would be consistent with how we treat pivot block.

Are we expecting that the number of files would be just too unwieldy?

@@ -33,7 +33,9 @@ public enum KeyValueSegmentIdentifier implements SegmentIdentifier {
GOQUORUM_PRIVATE_STORAGE(new byte[] {12}),
BACKWARD_SYNC_HEADERS(new byte[] {13}),
BACKWARD_SYNC_BLOCKS(new byte[] {14}),
BACKWARD_SYNC_CHAIN(new byte[] {15});
BACKWARD_SYNC_CHAIN(new byte[] {15}),
SNAPSYNC_MISSING_ACCOUNT_RANGE(new byte[] {16}),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should mark this PR as breaking backward compatibility if we add these column families

@@ -30,6 +30,7 @@
import java.util.function.Predicate;
import java.util.stream.Stream;

import kotlin.Pair;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use java 17 record types yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose to use org.apache.commons.lang3.tuple.Pair; is it fine for you ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we are using java 11

Copy link
Contributor

Choose a reason for hiding this comment

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

apache commons is good by me. We are so close to being able to use record types though... 🤤

Comment on lines 114 to 119
try (final Stream<Pair<byte[], byte[]>> entry = keyValueStorage.stream()) {
entry.forEach(
key -> {
lock.lock();
try {
if (!inUseCheck.test(key) && keyValueStorage.tryDelete(key)) {
if (!inUseCheck.test(key.getFirst()) && keyValueStorage.tryDelete(key.getFirst())) {
Copy link
Contributor

@garyschulte garyschulte Oct 24, 2022

Choose a reason for hiding this comment

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

could we not use streamKeys() ? It looks like we are only using Pair.getFirst() here and in other places. I suspect the iterator for just keys would be more performant in cases where we only want keys 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 132 to 136
public Stream<Pair<byte[], byte[]>> stream() {
final RocksIterator rocksIterator = db.newIterator();
rocksIterator.seekToFirst();
return RocksDbKeyIterator.create(rocksIterator).toStream();
return RocksDbIterator.create(rocksIterator).toStream();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we could offer both stream() and streamKeys()

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 catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@garyschulte
Copy link
Contributor

Are we expecting that the number of files would be just too unwieldy?

Maybe rocksdb transactions are just cleaner to deal with than files ? 🤔

@matkt
Copy link
Contributor Author

matkt commented Oct 25, 2022

Are we expecting that the number of files would be just too unwieldy?

Maybe rocksdb transactions are just cleaner to deal with than files ? 🤔

I tried a lot at the beginning to go through a file and I never managed to get a something clean. So I decided to switch to rocksdb .

Are we expecting that the number of files would be just too unwieldy?

Maybe rocksdb transactions are just cleaner to deal with than files ? 🤔

Yes, the number of accounts to fix is ​​something that cannot really be known over time. The bigger the chain, the bigger this list will be. And so you need a mechanism to manage this increase. I tried a lot to go through a file that splits when the size reaches a limit but after I said that I try to redo what rocksdb does better. So I decided to use rocksdb. But if you have a better idea to go through a file that could be split via a library why not

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM. The only open question is whether or not the new column families break backward compatibility. @gezero did you make besu handle extra column families or just bela?

@matkt matkt enabled auto-merge (squash) November 1, 2022 12:55
@matkt matkt merged commit da9b107 into hyperledger:main Nov 1, 2022
macfarla pushed a commit to macfarla/besu that referenced this pull request Jan 10, 2023
This PR avoids restarting the download of the world state from scratch when restarting Besu

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
This PR avoids restarting the download of the world state from scratch when restarting Besu

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants