feat(db): remove function for periodic backup of the RocksDB database#74
Open
feat(db): remove function for periodic backup of the RocksDB database#74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe pull request removes the periodic database backup feature: configuration, conditional bean wiring, AOP aspect, backup utility, related tests, and a test configuration file; all storage.backup keys and usages are deleted. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant BackupAspect as BackupRocksDBAspect
participant Args
participant BackupUtil as BackupDbUtil
participant SnapshotMgr as SnapshotManager
participant RocksDB
Manager->>BackupAspect: pushBlock(block)
BackupAspect->>Args: check dbBackupConfig & frequency
Note right of Args: decide whether backup is enabled\nand whether block.num % frequency == 0
Args-->>BackupAspect: decision
alt backup enabled & threshold met
BackupAspect->>BackupUtil: doBackup(block)
BackupUtil->>SnapshotMgr: list chain databases
SnapshotMgr-->>BackupUtil: DB instances
BackupUtil->>RocksDB: perform delete/copy per index
RocksDB-->>BackupUtil: success/error
BackupUtil-->>BackupAspect: result/log
else skip backup
BackupAspect-->>Manager: continue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
317787106
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/test/java/org/tron/common/storage/rocksdb/RocksDbDataSourceImplTest.java (1)
126-140:⚠️ Potential issue | 🟡 MinorRemove
backupAndDeletetest and its corresponding dead code methods.The
backup()anddeleteDbBakPath()methods have no production callers—they are invoked only by this test. Since the PR removes the database backup feature, these low-level RocksDB APIs appear to be dead code. Remove the test and consider removing the unused methods fromRocksDbDataSourceImplas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/storage/rocksdb/RocksDbDataSourceImplTest.java` around lines 126 - 140, Delete the JUnit test method backupAndDelete in RocksDbDataSourceImplTest and remove the corresponding unused production methods backup(...) and deleteDbBakPath(...) from the RocksDbDataSourceImpl class (and any trivial helpers that only support them); ensure you also remove any imports or test utilities used solely by that test and run tests to confirm no other callers reference backup(...) or deleteDbBakPath(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@framework/src/test/java/org/tron/common/storage/rocksdb/RocksDbDataSourceImplTest.java`:
- Around line 126-140: Delete the JUnit test method backupAndDelete in
RocksDbDataSourceImplTest and remove the corresponding unused production methods
backup(...) and deleteDbBakPath(...) from the RocksDbDataSourceImpl class (and
any trivial helpers that only support them); ensure you also remove any imports
or test utilities used solely by that test and run tests to confirm no other
callers reference backup(...) or deleteDbBakPath(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c667cd3-3c5e-41cb-9a1d-45e49ddc5bfa
📒 Files selected for processing (13)
common/src/main/java/org/tron/common/config/DbBackupConfig.javacommon/src/main/java/org/tron/common/parameter/CommonParameter.javaframework/src/main/java/org/tron/core/config/DefaultConfig.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/ConfigKey.javaframework/src/main/java/org/tron/core/db/backup/BackupDbUtil.javaframework/src/main/java/org/tron/core/db/backup/BackupRocksDBAspect.javaframework/src/main/java/org/tron/core/db/backup/NeedBeanCondition.javaframework/src/test/java/org/tron/common/storage/rocksdb/RocksDbDataSourceImplTest.javaframework/src/test/java/org/tron/core/db/backup/BackupDbUtilTest.javaframework/src/test/java/org/tron/program/SupplementTest.javaframework/src/test/resources/config-test-dbbackup.confplugins/src/test/java/org/tron/plugins/DbLiteTest.java
💤 Files with no reviewable changes (12)
- plugins/src/test/java/org/tron/plugins/DbLiteTest.java
- common/src/main/java/org/tron/common/parameter/CommonParameter.java
- framework/src/test/java/org/tron/program/SupplementTest.java
- framework/src/main/java/org/tron/core/db/backup/NeedBeanCondition.java
- framework/src/main/java/org/tron/core/config/args/Args.java
- framework/src/test/resources/config-test-dbbackup.conf
- framework/src/main/java/org/tron/core/config/DefaultConfig.java
- framework/src/main/java/org/tron/core/db/backup/BackupRocksDBAspect.java
- framework/src/test/java/org/tron/core/db/backup/BackupDbUtilTest.java
- framework/src/main/java/org/tron/core/config/args/ConfigKey.java
- common/src/main/java/org/tron/common/config/DbBackupConfig.java
- framework/src/main/java/org/tron/core/db/backup/BackupDbUtil.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
317787106
changed the title
317787106
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Removes the periodic database backup feature (
storage.backup) entirely. This includes the backup configuration class, the AspectJ aspect that interceptedpushBlockto trigger copies, the backup utility, and all associated test fixtures and config files.Closes tronprotocol#6595.
Why are these changes required?
The
storage.backupfeature was designed to guard against data corruption from disk failures or abrupt process termination (e.g.kill -9). It has since become more harmful than helpful:kill -9does not corrupt RocksDB, which was the primary motivation for the feature.node.backup(UDP keep-alive failover). If one node becomes unavailable, traffic can be switched to the standby without any file-copy disruption.Changes:
DbBackupConfig,BackupDbUtil,BackupRocksDBAspect,NeedBeanCondition(common/framework).CommonParameter.dbBackupConfigfield andArgs.initRocksDbBackupProperty()initialiser.ConfigKey.STORAGE_BACKUP_*constants and the conditionalbackupRocksDBAspectSpring bean fromDefaultConfig.BackupDbUtilTestandconfig-test-dbbackup.conf; updatedRocksDbDataSourceImplTestandSupplementTestto useTEST_CONF.storage.backupblock is no longer read and has no effect. Nodes carrying this section in their config file can leave it in place without error, but it will be silently ignored.Backward Compatibility: Not compatible with v4.8.1 or older.
This PR has been tested by:
Follow up
Operators who currently rely on
storage.backupshould migrate to thenode.backupprimary–backup failover configuration.Extra details
N/A
Summary by CodeRabbit
Release Notes
Chores
Tests