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

Default storage format now Bonsai #6536

Merged
merged 23 commits into from
Feb 16, 2024
Merged

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Feb 7, 2024

Builds on #6530
Change default storage format to BONSAI (was FOREST)

Note - if you had previously used FOREST, this is a breaking change - at startup you will get an error indicating the mismatch -

Mismatch: DB at '/your-path' is FOREST (Version 1) but config expects BONSAI (Version 2). Please check your config

Refs #5391

Copy link

github-actions bot commented Feb 7, 2024

  • 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
  • I thought about running CI.
  • If I did not run CI, I ran as much locally as possible before pushing.

@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 9, 2024
@fab-10
Copy link
Contributor

fab-10 commented Feb 15, 2024

What happens if you are not specifying the format before and after the upgrade to this version, is the previous Forest format recognized or there is an format mismatch error?

@macfarla
Copy link
Contributor Author

What happens if you are not specifying the format before and after the upgrade to this version, is the previous Forest format recognized or there is an format mismatch error?

Good callout - Added to the description of the PR and the changelog

Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla enabled auto-merge (squash) February 16, 2024 06:28
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, just will prefer to have the tests use the new default instead of keeping using Forest

CHANGELOG.md Outdated Show resolved Hide resolved
params.addAll(
DataStorageOptions.fromConfig(
ImmutableDataStorageConfiguration.builder()
.from(DataStorageConfiguration.DEFAULT_FOREST_CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be better to use BONSAI in tests since it is the default?

@@ -201,6 +202,7 @@ public void startNode(final BesuNode node) {
.nodeKey(new NodeKey(new KeyPairSecurityModule(KeyPairUtil.loadKeyPair(dataDir))))
.metricsSystem(metricsSystem)
.transactionPoolConfiguration(txPoolConfig)
.dataStorageConfiguration(DataStorageConfiguration.DEFAULT_FOREST_CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@macfarla
Copy link
Contributor Author

That's a fair comment on the tests @fab-10 - I'm not going to get that done today though - do you reckon that could be a separate PR?

@fab-10
Copy link
Contributor

fab-10 commented Feb 16, 2024

That's a fair comment on the tests @fab-10 - I'm not going to get that done today though - do you reckon that could be a separate PR?

Yes. definitely it could be done in another PR

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, just the db mismatch warning message could be improved

@macfarla
Copy link
Contributor Author

That's a fair comment on the tests @fab-10 - I'm not going to get that done today though - do you reckon that could be a separate PR?

Yes. definitely it could be done in another PR

created #6583

macfarla and others added 2 commits February 16, 2024 20:25
Co-authored-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla merged commit 24f9201 into hyperledger:main Feb 16, 2024
50 checks passed
suraneti pushed a commit to suraneti/besu that referenced this pull request Feb 24, 2024
* default bonsai
* be explicit about config in tests
* default forest storage for fast sync tests
* added upcoming breaking changes section
* added db mismatch message

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla deleted the bonsai-default branch February 27, 2024 05:21
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Feb 27, 2024
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