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

feat(rocksdb): remove getters for internal rocksdb handles, expose backup instead #2535

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

rymnc
Copy link
Member

@rymnc rymnc commented Jan 6, 2025

Linked Issues/PRs

Description

  • creates a feature backup and 2 functions, backup and restore.
  • not too many config options available at the moment
  • bubbled it to CombinedDatabase, similarly to the prune api from rocksdb.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc self-assigned this Jan 7, 2025
@rymnc rymnc changed the title chore(rocksdb): remove getters for internal rocksdb handles chore(rocksdb): remove getters for internal rocksdb handles, expose backup instead Jan 7, 2025
@rymnc rymnc requested a review from Copilot January 7, 2025 12:01

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • crates/fuel-core/src/state/rocks_db.rs: Evaluated as low risk
  • CHANGELOG.md: Evaluated as low risk
  • crates/fuel-core/src/state/historical_rocksdb.rs: Evaluated as low risk
Comments suppressed due to low confidence (4)

crates/fuel-core/src/combined_database.rs:89

  • The error message could be more descriptive. Consider including more context about the failure.
crate::database::Error::BackupError(anyhow::anyhow!("Failed to create temporary backup directory: {}", e))

crates/fuel-core/src/combined_database.rs:163

  • The error message could be more descriptive. Consider including more context about the failure.
crate::database::Error::RestoreError(anyhow::anyhow!("Failed to create temporary restore directory: {}", e))

crates/fuel-core/src/combined_database.rs:133

  • Ensure that temporary directories are properly removed in case of errors to avoid leaving residue.
std::fs::remove_dir_all(temp_backup_dir.path()).map_err(|e| {

crates/fuel-core/src/combined_database.rs:207

  • Ensure that temporary directories are properly removed in case of errors to avoid leaving residue.
std::fs::remove_dir_all(temp_restore_dir.path()).map_err(|e| {
@rymnc rymnc marked this pull request as ready for review January 7, 2025 12:10
@rymnc rymnc requested a review from a team January 7, 2025 12:10
@rymnc rymnc changed the title chore(rocksdb): remove getters for internal rocksdb handles, expose backup instead feat(rocksdb): remove getters for internal rocksdb handles, expose backup instead Jan 7, 2025
@rymnc rymnc added the release label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant