HBASE-29524 Handle bulk-loaded HFiles in delete and cleanup process #7239
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| for (FileStatus item : bulkContents) { | ||
| fs.delete(item.getPath(), true); // recursive delete of each child | ||
| } | ||
| System.out.println("Deleted all contents under Bulk Load directory: " + bulkloadDir); |
There was a problem hiding this comment.
Is there a specific reason you chose to loop and delete the contents individually?
I think a single recursive fs.delete() here would reduce RPCs (like in line 1072 in deleteOldWALFiles())
There was a problem hiding this comment.
The main reason is to preserve the parent directories. In this case, I want to delete all the WAL directories and bulkload directories, but still keep the root directory intact. This way, when we restart the continuous backup to the same backup directory, the required directory structure will already be in place.
There was a problem hiding this comment.
if the required directory isn't exist, e.g. the first using backup, what would that be ? will we create it?
the problem is that this list and per-call in s3 could be experience , that's why S3A has the feature of fs.s3a.multiobjectdelete.enable (with fs.delete path recursively), if we do this way here, and if the list of bulkloaded files are huge within the bulkload directory, you will hit s3 throttling very easily, and we will need to write/folk the same implementation here.
so, instead of having the same required directory structure in place, I suggested you handle it as an enable/reenable check for the required directory structure, and create it to avoid large mount of per-object delete call.
There was a problem hiding this comment.
One clarification from my side:
Our current backup directory structure looks like this:
-- wal_backup_directory/
-- WALs/
-- 23-08-2025/
... wal files
-- 24-08-2025/
-- 25-08-2025/
-- bulk-load-files/
-- 23-08-2025/
... bulkload files
-- 24-08-2025/
-- 25-08-2025/
As you can see, when we loop and delete, it’s done at the day-wise directory level, not at the individual file level. So the number of delete operations is relatively small.
Regarding re-creation: when we enable a replication peer, there isn’t any placeholder mechanism to re-create this directory structure. In the enable/disable flow, everything remains running, the only difference is that replication is paused and then resumed. Once enabled again, entries are sent to the replication endpoint directly.
It’s only during a restart that things are different. On restart, we instantiate the ContinuousBackupReplicationEndpoint again, and in its constructor we can re-create the directory structure if it’s missing. With enable/disable, this re-creation step isn’t possible.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the backup cleanup process to handle bulk-loaded HFiles alongside WAL files during delete and cleanup operations. The implementation ensures that both WAL and bulk-load directories are managed consistently throughout the backup lifecycle.
Key changes:
- Extended cleanup commands to process bulk-load directories in addition to WAL directories
- Updated test infrastructure to create and verify both WAL and bulk-load folder structures
- Added comprehensive verification to ensure both directory types are properly cleaned up
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| TestBackupCommands.java | Updated test to verify cleanup of bulk-load directories alongside WAL directories |
| TestBackupDeleteWithCleanup.java | Enhanced test setup and verification methods to handle both WAL and bulk-load directories |
| BackupCommands.java | Modified cleanup logic to delete old bulk-load files in addition to WAL files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java
Outdated
Show resolved
Hide resolved
| if (dayStart + ONE_DAY_IN_MILLISECONDS - 1 < cutoffTime) { | ||
| System.out.println("Deleting outdated WAL directory: " + dirPath); | ||
| fs.delete(dirPath, true); | ||
| fs.delete(new Path(bulkloadDir, dirName), true); |
There was a problem hiding this comment.
[nitpick] Consider adding a corresponding print statement for bulk-load directory deletion, similar to line 1070 for WAL directories, to maintain consistent logging behavior.
| fs.delete(new Path(bulkloadDir, dirName), true); | |
| Path bulkloadPath = new Path(bulkloadDir, dirName); | |
| System.out.println("Deleting corresponding bulk-load directory: " + bulkloadPath); | |
| fs.delete(bulkloadPath, true); |
There was a problem hiding this comment.
IMO this comment is right, please change the code accordingly
| for (FileStatus item : bulkContents) { | ||
| fs.delete(item.getPath(), true); // recursive delete of each child | ||
| } | ||
| System.out.println("Deleted all contents under Bulk Load directory: " + bulkloadDir); |
There was a problem hiding this comment.
if the required directory isn't exist, e.g. the first using backup, what would that be ? will we create it?
the problem is that this list and per-call in s3 could be experience , that's why S3A has the feature of fs.s3a.multiobjectdelete.enable (with fs.delete path recursively), if we do this way here, and if the list of bulkloaded files are huge within the bulkload directory, you will hit s3 throttling very easily, and we will need to write/folk the same implementation here.
so, instead of having the same required directory structure in place, I suggested you handle it as an enable/reenable check for the required directory structure, and create it to avoid large mount of per-object delete call.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…7239) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
…7239) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kota-SH <shanmukhaharipriya@gmail.com>
4 participants
No description provided.