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

Add backup create/list fucntions for rocksdb and postgres #206

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

kimhanbeom
Copy link
Contributor

This feature is intended for temporary use, driven by project requirements.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 244 lines in your changes are missing coverage. Please review.

Comparison is base (eeae1e0) 22.70% compared to head (3c85951) 22.25%.

Files Patch % Lines
src/backup/postgres.rs 0.00% 167 Missing ⚠️
src/backup.rs 0.00% 66 Missing ⚠️
src/tables.rs 0.00% 8 Missing ⚠️
src/lib.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           backup     #206      +/-   ##
==========================================
- Coverage   22.70%   22.25%   -0.45%     
==========================================
  Files          78       79       +1     
  Lines       12377    12621     +244     
==========================================
- Hits         2810     2809       -1     
- Misses       9567     9812     +245     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syncpark syncpark merged commit 20763fb into petabi:backup Feb 7, 2024
4 of 6 checks passed
@msk
Copy link
Contributor

msk commented Feb 7, 2024

Given the introduction of this temporary feature, I have a couple of questions:

  1. Could you elaborate on why this new feature is considered temporary? Isn't this what Enhance backup::create to support PostgreSQL backup #71, Enhance backup::list Function to Include PostgreSQL Backups #76, and Enhance backup::restore function to support PostgreSQL in addition to RocksDB #80 are for?

  2. Regarding the original PR ([WIP] Enhance backup logic #105), which is still marked as WIP and has unresolved conflicts with the main branch, what would be the best course of action? Should we consider integrating aspects of the new temporary feature into it, or do you envision a different path forward for resolving the WIP PR, such as closing it or refactoring it in light of recent developments?

@kimhanbeom
Copy link
Contributor Author

kimhanbeom commented Feb 21, 2024

@msk

Currently, an external project requires backup-related functionality with the following conditions

  1. create rocksdb/postgres backup archive. (the created backup archive should be externally transferable and independently available on another server/current server, i.e. rocksdb full backup is required)
  2. output a list of backup archives.

Currently, the backup implemented in review-database only supports incremental backup, so it is not possible to create a full backup of a specific point. Therefore, in order to satisfy the above two conditions, I implemented a feature to create a backup through checkpoint rather than backup engine.

However, in order to use the backup feature of rocksdb more reliably, I think that we should use rocksdb backup engine. That's why I'm thinking of raising an issue/pr for the feature so that the backup engine in the rocks crate I'm currently using supports full backup, and after that feature is added, I'll re-implement the backup feature and include it in main.

For the existing WIP pr, I'm going to close it because I think I just need to reference some features that I can reuse.

@msk
Copy link
Contributor

msk commented Feb 21, 2024

@kimhanbeom, thank you for the detailed update. It sounds good to me.

@msk msk mentioned this pull request 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