-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-13009. Background snapshot defrag service (Draft v1) #9117
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
Conversation
swamirishi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smengcl thank you for starting to work on the framework for defrag service. I believe the approach to defrag a snapshot is not the one in the design doc and this could be suboptimal. I have left comments inline
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
| throw new RocksDatabaseException("Failed to create temporary SST directory: " + tempSstDir, e); | ||
| } | ||
|
|
||
| LOG.info("Applied {} total incremental changes using snapshotDiff approach", totalChanges); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW we should truncate other tables like deletedTable, deletedDirectoryTable, snapshotRenamedTable, bucketTable, volumeTable etc. and directly bulk import the tables into the defragged snapshot DB.
Here we can use: dumpToFile and loadFromFile which would just dump the table into an sstFile which can be directly loaded. This would be much more optimal.
ozone/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java
Lines 469 to 477 in ae92a18
| @Override | |
| public void dumpToFileWithPrefix(File externalFile, KEY prefix) throws RocksDatabaseException, CodecException { | |
| rawTable.dumpToFileWithPrefix(externalFile, encodeKey(prefix)); | |
| } | |
| @Override | |
| public void loadFromFile(File externalFile) throws RocksDatabaseException { | |
| rawTable.loadFromFile(externalFile); | |
| } |
jojochuang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would need tests for the new service.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a Background Snapshot Defragmentation Service for Ozone, designed to optimize snapshot storage by defragmenting RocksDB instances containing fragmented data. The service processes snapshots sequentially in the active chain, performing full defragmentation for the first snapshot and incremental defragmentation for subsequent ones based on snapshot diffs.
Key changes:
- Introduces
SnapshotDefragServicefor background snapshot defragmentation - Adds configuration keys and service integration into the key management framework
- Modifies RocksDB utilities to support SST file operations and database access for defragmentation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| SnapshotDefragService.java | Main service implementation for defragmenting snapshots |
| KeyManagerImpl.java | Integration of defrag service into key management lifecycle |
| KeyManager.java | Interface addition for defrag service access |
| OMConfigKeys.java | Configuration keys for defrag service parameters |
| RocksDatabase.java | Exposed methods for checkpoint creation and data access |
| RDBSstFileWriter.java | Added delete operation and made class public |
| OzoneConsts.java | Added constant for defragmented checkpoint directory |
| OzoneConfigKeys.java | Timeout configuration for defrag service |
Comments suppressed due to low confidence (1)
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java:1
- The configuration is disabled by default pending upgrade handling completion. This indicates the feature may not be ready for production use and the upgrade compatibility should be addressed.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Outdated
Show resolved
Hide resolved
ofc, as I mentioned in the description, the test case was going to be cherry-picked later from the POC. The service alone is over 1000 lines diff. I wanted that to be commented on first. :) |
Conflicts: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java
… be handled in upper logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 17 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
...ration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDefragService.java
Show resolved
Hide resolved
...ration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
Show resolved
Hide resolved
| </property> | ||
| <property> | ||
| <name>ozone.snapshot.defrag.limit.per.task</name> | ||
| <value>1</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to benchmark to find a more appropriate value.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotDefragService.java
Show resolved
Hide resolved
|
I have addressed the majority of the comments in this PR except 3:
This draft will be closed in favor of a new PR #9227, because of the rebase as a result of merging #9133 and other improvements. |
What changes were proposed in this pull request?
Implement Background Snapshot Defragmentation Service outlined in the design
Some commits are cherry-picked from the POC and rebased/changed.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13009
How was this patch tested?