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

refactor(db)!: remove support for cleveldb, boltdb, rocksdb, badgerdb #974

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

lklimek
Copy link
Collaborator

@lklimek lklimek commented Nov 5, 2024

Issue being fixed or feature implemented

Tenderdash uses only goleveldb database backend. However, we have support for other database backends, like cleveldb, boltdb, rocksdb, badgerdb, that are not used. One of these backends, badgerdb, causes issues in tests due to compilation error.

In addition,only goleveldb is considered as stable by upstream project - https://github.com/cometbft/cometbft-db .

What was done?

Dropped support for unused database engines: cleveldb, boltdb, rocksdb, badgerdb.

How Has This Been Tested?

GHA pipeline

Breaking Changes

Dropped support of cleveldb, boltdb, rocksdb, badgerdb (as far as we know, these are not used anyway).

Checklist:

  • [x ] I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

Release Notes

  • New Features

    • Simplified database configuration: Only goleveldb is now supported across various components.
    • Updated command-line options for node commands to reflect the removal of multiple database backends.
  • Documentation

    • Enhanced clarity in configuration documentation, emphasizing the use of goleveldb.
    • Updated README files for E2E testing with clearer instructions and formatting improvements.
  • Bug Fixes

    • Improved validation logic for database options in configuration files and structs to enforce the new restrictions.

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Walkthrough

The pull request modifies multiple files to streamline the Tendermint build and configuration process by removing support for several database backends, specifically cleveldb, badgerdb, rocksdb, and boltdb. Changes include updates to the Makefile, command-line flags, configuration validation, and documentation, ensuring that only goleveldb is supported. The overall structure of the affected files remains intact, with a focus on simplifying the configuration options and enhancing validation logic.

Changes

File Change Summary
Makefile Removed conditional blocks for cleveldb, badgerdb, rocksdb, and boltdb from build options.
cmd/tenderdash/commands/inspect.go Removed --db-backend command-line flag from MakeInspectCommand.
cmd/tenderdash/commands/run_node.go Removed db-backend flag from AddNodeFlags function.
config/config.go Updated DBBackend comment to state only goleveldb is supported; added validation in ValidateBasic.
config/toml.go Updated comments in defaultConfigTemplate to reflect only goleveldb as supported.
docs/nodes/configuration.md Simplified documentation regarding database backends, highlighting only goleveldb.
test/e2e/Makefile Updated node target build command to include only deadlock as a build tag.
test/e2e/README.md Minor formatting and clarity adjustments in E2E testing documentation.
test/e2e/generator/generate.go Updated nodeDatabases variable to include only goleveldb.
test/e2e/networks/*.toml Changed database types for multiple nodes from various backends to goleveldb.
test/e2e/pkg/manifest.go Updated documentation in ManifestNode struct to specify only goleveldb is supported.
test/e2e/pkg/testnet.go Restricted Database field in Node struct to only allow goleveldb and memdb.

Possibly related PRs

  • build(deps): go 1.23, mockery 2.46.2, golangci-lint 1.61 #954: The changes in this PR involve updating the Go version to 1.23, which aligns with the main PR's modifications to the Makefile and other files that streamline the build process by removing specific database options. Both PRs reflect a shift towards a simplified configuration and dependency management.

Suggested reviewers

  • shumkov
  • QuantumExplorer

Poem

🐇 In the meadow where code does play,
A rabbit hops with joy today.
"Goleveldb is the way to go,
Simplified paths, let the build flow!
With flags removed, our code is neat,
Hopping along, oh what a treat!" 🐇


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a441b9a and 8661170.

📒 Files selected for processing (1)
  • config/config.go (3 hunks)
🔇 Additional comments (2)
config/config.go (2)

201-206: LGTM! Clear and accurate documentation update.

The comment changes accurately reflect:

  1. The removal of support for other database backends
  2. The continued support for memdb in tests

347-354: LGTM! Validation logic is correct and comprehensive.

The validation implementation:

  1. Correctly restricts database backend to goleveldb
  2. Properly handles memdb for test environments
  3. Provides clear error messages

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@lklimek
Copy link
Collaborator Author

lklimek commented Nov 5, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
test/e2e/networks/dashcore.toml (1)

Line range hint 1-4: Consider documenting test coverage strategy

While the configuration maintains comprehensive test coverage across different scenarios (ABCI protocols, sync modes, perturbations), consider adding a comment block explaining the test coverage strategy. This would help maintainers understand the purpose of each node's configuration.

 # This testnet is run by CI, and attempts to cover a broad range of
 # functionality with a single network.
+#
+# Test coverage matrix:
+# - validator02: TCP ABCI + immediate persistence
+# - validator03: Block retention + periodic persistence + kill recovery
+# - validator04: Builtin ABCI + pause recovery
+# - validator05: Late joining + block sync + state sync + multiple perturbations
test/e2e/networks/rotate.toml (1)

123-146: Consider updating test documentation.

Since this test configuration previously validated multiple database backends, it would be helpful to update the test documentation to clarify that database backend variation is no longer part of the test coverage.

Would you like me to help draft documentation updates to reflect these changes?

docs/nodes/configuration.md (3)

51-51: Enhance database backend documentation.

While the documentation correctly lists goleveldb as the supported backend, consider adding:

  1. An explicit note that other backends (cleveldb, boltdb, rocksdb, badgerdb) are no longer supported
  2. Migration instructions for users currently using the removed backends
-# Database backend: goleveldb 
-# * goleveldb (github.com/syndtr/goleveldb - most popular implementation)
-#   - pure go
-#   - stable
+# Database backend: goleveldb (only supported option)
+# * goleveldb (github.com/syndtr/goleveldb - most popular implementation)
+#   - pure go
+#   - stable
+#
+# Note: Support for cleveldb, boltdb, rocksdb, and badgerdb has been removed.
+# If you were using any of these backends, please migrate your data to goleveldb.
+# Migration guide: [TODO: Add link to migration guide]

535-537: Fix formatting inconsistency in new parameters section.

The formatting of the new parameters section is inconsistent with the rest of the document.

-# There are three new parameters, which are enabled if use-legacy is set to false.
-
-# queue-type = sets a type of queue to use in the p2p layer. There are three options available `fifo`, `priority` and `wdrr`. The default is priority
-# bootstrap-peers = is a list of comma seperated peers which will be used to bootstrap the address book.
-# max-connections = is the max amount of allowed inbound and outbound connections.
+### New Parameters
+
+These parameters are enabled when `use-legacy` is set to false:
+
+- `queue-type` = sets the type of queue to use in the p2p layer. Available options: `fifo`, `priority`, and `wdrr`. Default: `priority`
+- `bootstrap-peers` = a comma-separated list of peers used to bootstrap the address book
+- `max-connections` = maximum number of allowed inbound and outbound connections

580-580: Enhance PostgreSQL schema setup documentation.

The PostgreSQL schema setup command could be more detailed and properly formatted.

-psql ... -f state/indexer/sink/psql/schema.sql
+# Apply the PostgreSQL schema (replace the connection parameters with your values)
+psql -h <hostname> -p <port> -U <username> -d <database> -f state/indexer/sink/psql/schema.sql
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dbe5cf8 and a441b9a.

📒 Files selected for processing (15)
  • Makefile (0 hunks)
  • cmd/tenderdash/commands/inspect.go (0 hunks)
  • cmd/tenderdash/commands/run_node.go (0 hunks)
  • config/config.go (2 hunks)
  • config/toml.go (1 hunks)
  • docs/nodes/configuration.md (3 hunks)
  • test/e2e/Makefile (1 hunks)
  • test/e2e/README.md (2 hunks)
  • test/e2e/generator/generate.go (0 hunks)
  • test/e2e/networks/ci.toml (3 hunks)
  • test/e2e/networks/dashcore.toml (2 hunks)
  • test/e2e/networks/island.toml (2 hunks)
  • test/e2e/networks/rotate.toml (1 hunks)
  • test/e2e/pkg/manifest.go (1 hunks)
  • test/e2e/pkg/testnet.go (1 hunks)
💤 Files with no reviewable changes (4)
  • Makefile
  • cmd/tenderdash/commands/inspect.go
  • cmd/tenderdash/commands/run_node.go
  • test/e2e/generator/generate.go
✅ Files skipped from review due to trivial changes (1)
  • test/e2e/README.md
🔇 Additional comments (15)
test/e2e/networks/island.toml (3)

2-2: LGTM: Formatting improvement

The formatting change improves readability while maintaining the same functionality.


52-52: Consistent database backend configuration across all validators

All validator nodes have been properly updated to use "goleveldb" as their database backend, maintaining consistency across the test network configuration.

Also applies to: 59-59, 66-66, 72-72, 78-78


45-45: Database backend standardization looks good

The change to "goleveldb" aligns with the PR objective to standardize database backends.

Let's verify there are no remaining references to other database backends in the e2e test configurations:

✅ Verification successful

Database backend standardization verified

All database configurations in the e2e test files have been standardized to use "goleveldb". The verification found:

  • All explicit database configurations in network TOML files (island.toml, ci.toml, rotate.toml, dashcore.toml) use "goleveldb"
  • No other database backend references were found in the remaining e2e test configuration files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to removed database backends in e2e test configs
echo "Checking for removed database backends in e2e test configs..."
rg -g '*.toml' "database.*=.*(cleveldb|boltdb|rocksdb|badgerdb)" test/e2e/

Length of output: 209


Script:

#!/bin/bash
# Let's first check all database configurations in e2e test configs
echo "Checking all database configurations in e2e test configs..."
rg -g '*.toml' "database.*=.*" test/e2e/

# Also check if there are any other TOML files that might contain database configs
echo -e "\nListing all TOML files in test/e2e/..."
fd -e toml . test/e2e/

Length of output: 1516


Script:

#!/bin/bash
# Let's check the remaining TOML files that didn't show up in the database search
echo "Checking database configurations in simple.toml, single.toml, built-in.toml, and socket.toml..."
rg "database" test/e2e/networks/simple.toml test/e2e/networks/single.toml test/e2e/node/built-in.toml test/e2e/node/socket.toml

Length of output: 328

test/e2e/Makefile (1)

29-29: LGTM! Build tag simplification aligns with PR objectives.

The removal of database-related build tags (badgerdb,boltdb,cleveldb) while retaining deadlock is consistent with the PR's goal of removing support for unused database backends.

Let's verify that no other database-related build tags exist in the codebase:

✅ Verification successful

Build tag removal is safe and consistent with codebase changes

The verification confirms that:

  1. The database-related config files are test fixtures in internal/libs/confix/testdata/, as evidenced by their location and the associated test file confix_test.go.
  2. No database backend implementations remain in the codebase.
  3. No imports of the removed database packages exist.
  4. The removal of database-related build tags from the Makefile aligns with the complete removal of these database backends.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining database-related build tags
# Expected: No occurrences of removed database build tags

echo "Checking for database-related build tags in go files..."
rg -l "//\s*\+build.*(?:badger|bolt|clevel|rock)db" 

echo "Checking for database-related tags in build commands..."
rg -l "\-tags.*(?:badger|bolt|clevel|rock)db"

Length of output: 619


Script:

#!/bin/bash
# Let's check the content of these config files to understand if they are just test fixtures
# or actual configuration files that need updating

echo "Checking content of config files:"
for file in internal/libs/confix/testdata/v*-config.toml; do
    echo "=== $file ==="
    grep -A 5 -B 5 "tags" "$file"
done

# Also verify if there are any other references to these databases in the codebase
echo -e "\nChecking for any other database references:"
rg -l "(?i)(badger|bolt|clevel|rock)db" --type go

Length of output: 12950


Script:

#!/bin/bash
# Let's verify if these are just test fixtures by checking their location and usage
echo "Checking if these are test fixtures:"
rg -l "testdata" --type go internal/libs/confix/

# Also check if there are any actual database backend implementations remaining
echo -e "\nChecking for database implementation files:"
fd -e go -x grep -l "type.*DB struct" {}

# Check for any remaining database imports
echo -e "\nChecking for database-related imports:"
rg -l '(?i)import.*".*/(badger|bolt|levigo|rocksdb)"'

Length of output: 464

test/e2e/networks/ci.toml (1)

44-44: Database backend standardization looks good!

The changes consistently update all nodes to use goleveldb as the database backend, which aligns with the PR objective to remove support for alternative database implementations.

Let's verify that no old database backends remain in the configuration:

Also applies to: 52-52, 62-62, 70-70, 79-79

✅ Verification successful

Database backend standardization successfully verified

All database configurations across the test networks (ci.toml, island.toml, rotate.toml, dashcore.toml) have been standardized to use "goleveldb". No instances of removed database backends (cleveldb, boltdb, rocksdb, badgerdb) were found in any TOML configuration files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to removed database backends
# Expected: No matches should be found

echo "Checking for removed database backends in TOML files..."
rg -i "database.*=.*\"(cleveldb|boltdb|rocksdb|badgerdb)\"" --type toml

Length of output: 192


Script:

#!/bin/bash
# Let's verify all database configurations in TOML files
# First, find all database configurations
echo "All database configurations in TOML files:"
rg -i "database.*=.*\".*\"" --type toml

# Also check the specific file mentioned in the review
echo -e "\nSpecific file content:"
cat test/e2e/networks/ci.toml

Length of output: 2995

test/e2e/networks/dashcore.toml (4)

59-59: LGTM: Database change aligns with standardization effort

The change to goleveldb for validator02 maintains test coverage for TCP ABCI protocol scenarios with immediate persistence.


67-67: LGTM: Maintains critical test coverage

The switch to goleveldb for validator03 preserves important test scenarios including block retention, periodic persistence, and kill recovery behavior.


77-77: LGTM: Preserves builtin ABCI protocol testing

The database change for validator04 maintains test coverage for goleveldb with the builtin ABCI protocol implementation.


85-85: LGTM: Maintains comprehensive test scenarios

The change to goleveldb for validator05 preserves critical test coverage for late-joining validators, block sync, state sync, and multiple failure modes.

test/e2e/networks/rotate.toml (1)

123-123: Database backend changes align with PR objectives.

The changes consistently update all validator nodes to use goleveldb, removing the previous test coverage for boltdb, badgerdb, and cleveldb backends. This aligns with the PR's goal of standardizing on goleveldb.

Let's verify there are no remaining references to the removed database backends in the test configurations:

Also applies to: 131-131, 139-139, 146-146

✅ Verification successful

The previous search might have failed because the files were not present or the pattern was too specific. Let's try a broader search to see all database configurations in TOML files to verify the migration is complete.


Database backend migration successfully verified

All database configurations in test TOML files (rotate.toml, island.toml, dashcore.toml, and ci.toml) consistently use "goleveldb" as the backend, confirming complete migration away from other database backends.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to removed database backends in test configurations
rg -g '*.toml' "boltdb|badgerdb|cleveldb|rocksdb" test/

Length of output: 57


Script:

#!/bin/bash
# Let's verify all database configurations in TOML files to ensure complete migration
rg -g '*.toml' 'database\s*=\s*"[^"]+"' test/

Length of output: 1042

test/e2e/pkg/manifest.go (1)

135-136: LGTM! Documentation updated to reflect database backend changes.

The documentation update correctly reflects that only "goleveldb" is now supported, which aligns with the PR objective to remove support for other database backends.

Let's verify that all E2E test configurations are using "goleveldb":

✅ Verification successful

All E2E test configurations are already using "goleveldb" as the database backend

The verification confirms that all database configurations in the E2E test files (test/e2e/networks/*.toml) are consistently set to "goleveldb". This aligns perfectly with the documentation update in test/e2e/pkg/manifest.go stating that only "goleveldb" is supported.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check all E2E test configurations to ensure they only use goleveldb
# Test: Search for any database configuration in test files. Expect: Only "goleveldb" or default values.

echo "Checking E2E test configurations for database settings..."
rg -g '*.toml' 'database.*=.*' test/e2e/

Length of output: 1162

test/e2e/pkg/testnet.go (2)

Line range hint 539-543: LGTM! Database validation aligns with PR objectives.

The validation logic correctly restricts database options to only "goleveldb" and "memdb", which aligns with the PR's goal of removing support for cleveldb, boltdb, rocksdb, and badgerdb backends.


Line range hint 539-543: Verify test configurations and documentation consistency.

Let's ensure there are no test configurations using the removed database backends and that documentation is updated accordingly.

config/toml.go (1)

104-107: Documentation update accurately reflects database backend changes.

The simplified documentation correctly states that only goleveldb is supported, which aligns with the PR's objective to remove support for other database backends.

Let's verify that references to removed database backends have been cleaned up:

✅ Verification successful

Let me try a different search pattern to verify the database backend implementation:


Documentation update accurately reflects database backend changes

The verification confirms that:

  1. The code consistently enforces goleveldb as the only supported database backend through validation in config/config.go
  2. All database-related code paths use goleveldb or memdb (for testing)
  3. No references to removed database backends (cleveldb, boltdb, rocksdb, badgerdb) remain in the codebase
  4. The documentation in config/toml.go correctly reflects the current state of the codebase

The changes are consistent across the entire codebase and properly documented.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to removed database backends
echo "Searching for references to removed database backends..."
rg -i "cleveldb|boltdb|rocksdb|badgerdb" --type go

# Search for database backend validation logic
echo "Checking database backend validation..."
ast-grep --pattern 'if $x == "goleveldb" { $$$
} else { $$$ }'

Length of output: 329


Script:

#!/bin/bash
# Search for database backend related code
echo "Searching for database backend related code..."
rg -i "database.*backend|db.*backend" --type go -A 3

# Search for database type validation
echo "Checking database type validation..."
rg -i "dbtype|db_type|database.*type" --type go -A 3

# Search for goleveldb references
echo "Checking goleveldb references..."
rg -i "goleveldb" --type go -A 3

Length of output: 15258

config/config.go (1)

200-205: LGTM! Clear documentation of supported database backend.

The comment accurately reflects that only goleveldb is supported while maintaining a note about memdb for testing purposes.

config/config.go Outdated Show resolved Hide resolved
@lklimek
Copy link
Collaborator Author

lklimek commented Nov 5, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Base automatically changed from deps/cometbft-tm-db to v1.3-dev November 6, 2024 08:02
@lklimek lklimek merged commit 52ea7e5 into v1.3-dev Nov 6, 2024
19 checks passed
@lklimek lklimek deleted the refactor/drop-db-support branch November 6, 2024 08:02
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.

2 participants