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

Database backend configuration #4828

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

RickiNano
Copy link
Contributor

In the current implementation, the database backend is determined by the setting of RocksDb.Enable config setting which is not very intuitive.
This PR replaces that setting with a new database_backend setting. It can be set to either lmdb or rocksdb. If not set, it defaults to lmdb.
To make it easier for node operators that are currently using RocksDb, the node will notify the operator with an error message if the config file has the old RocksDb.Enable set to true and database_backend set to lmdb (for example when using an old config file that does not have the new database_backend property). The error message can be removed in future versions when operators have migrated to use the new database_backend type
This PR also makes it easier to add more backends in the future.

@gr0vity-dev-bot
Copy link

gr0vity-dev-bot commented Jan 19, 2025

Test Results for Commit 4c856a4

Pull Request 4828: Results
Overall Status:

Test Case Results

Last updated: 2025-01-24 16:21:24 UTC

@pwojcikdev
Copy link
Contributor

I agree with doing the backend configuration this way, but it should not throw an error if legacy rocksdb.enable is set. There should be a warning, but the node should check for this case and use rocksdb, there is no reason not to do that. It's not a good user experience otherwise.

The only case where we should throw an error is if there is a mismatch between rocksdb.enable and database_backend, ie. one is explicitly set to rocksdb and the other to lmdb.

@RickiNano
Copy link
Contributor Author

The way we currently handle configuration errors are by logging an error an exit the application, so this is not really any different, imo.
If we don't let the users know that they have to update the config file, they will keep using the deprecated RocksDb.Enable without even knowing it. We have to get rid of this option in the config file somehow and use the new setting instead.

I only see two solutions to this:

  1. Notify the user and let them update the config themselves (as in this PR)
  2. Automatically update the config file on launch

Option 2 is definitely a possibility. We already have the --update_config option that migrates the current configuration into the latest one. I could change that function so that it also migrates the RocksDb.Enable into the appropriate database_backend setting. I feel that it is a bit controversial to change the users config file without permission, though.

nano/node/nodeconfig.cpp Outdated Show resolved Hide resolved
@RickiNano RickiNano requested a review from pwojcikdev January 24, 2025 15:32
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.

4 participants