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

BucketListDB enabled by default #4277

Merged
merged 1 commit into from
May 2, 2024

Conversation

SirTyson
Copy link
Contributor

@SirTyson SirTyson commented Apr 8, 2024

Description

Resolves #4202

This change defaults BucketListDB to enabled. The optional EXPERIMENTAL_BUCKETLIST_DB has been changed to the required flag DEPRECATED_SQL_LEDGER_STATE. If this flag is not set, core will fail to start. When BucketListDB is enabled for the first time, many SQL tables are dropped and can only be recovered by replaying many transactions. Crashing if this flag is not explicitly defined protects operators from accidentally dropping these tables when upgrading.

I'm not set on the name DEPRECATED_SQL_LEDGER_STATE, but I wanted to include "deprecated" in the name to discourage using SQL for ledger state as much as possible.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@SirTyson SirTyson force-pushed the bucketlist-db-default branch from 5dc9b00 to 6bda012 Compare April 26, 2024 22:45
@SirTyson
Copy link
Contributor Author

After some discussion (#4202), we are going to keep the experimental flags for the 21.0 release to not break compatibility with Horizon. We still want to require that some BucketListDB related flag is set on startup, so operators must either use the new DEPRECATED_SQL_LEDGER_STATE flag or the previous experimental flag. Additionally, we will support all the old EXPERIMENTAL_ parameters in addition to their new counterparts that have dropped the "EXPERIMENTAL" from the parameter name.

@SirTyson SirTyson marked this pull request as ready for review April 26, 2024 22:51
# DEPRECATED_SQL_LEDGER_STATE (bool) default false
# When set to true, SQL is used to store all ledger state instead of
# BucketListDB. This is not recommended and may cause performance degregradation.
# This is deprecated and will be removed in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add something about offers still being in SQL even with BucketListDB enabled?

# Determines whether BucketListDB indexes are saved to disk for faster
# startup. Should only be set to false for testing.
# Validators do not currently support persisted indexes. If NODE_IS_VALIDATOR=true,
# this value is ingnored and indexes are never persisted.
EXPERIMENTAL_BUCKETLIST_DB_PERSIST_INDEX = true
BUCKETLIST_DB_PERSIST_INDEX = true

# EXPERIMENTAL_BUCKETLIST_DB (bool) default false
Copy link
Contributor

Choose a reason for hiding this comment

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

EXPERIMENTAL_BACKGROUND_EVICTION_SCAN

bool EXPERIMENTAL_BUCKETLIST_DB;
// A config parameter that when set uses SQL as the primary
// key-value store for LedgerEntry lookups instead of BucketListDB.
bool DEPRECATED_SQL_LEDGER_STATE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this doesn't follow what was proposed in #4202 (comment), because you're removing EXPERIMENTAL_BUCKETLIST_DB from the API completely, which breaks operators who set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still support it in the config file itself, but it's just been renamed in the config class to a single name. See src/main/Config.cpp:1084.

@SirTyson SirTyson force-pushed the bucketlist-db-default branch from 6bda012 to a1de765 Compare May 2, 2024 17:41
@graydon
Copy link
Contributor

graydon commented May 2, 2024

r+ a1de765

@latobarita latobarita merged commit 84546ad into stellar:master May 2, 2024
15 checks passed
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.

Require BucketsDB flag in config file
4 participants