-
Notifications
You must be signed in to change notification settings - Fork 433
RI:7799: Stream bulk delete report #5278
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
Code Coverage - Integration Tests
|
Code Coverage - Backend unit tests
Test suite run success2988 tests passing in 287 suites. Report generated by 🧪jest coverage report action from 9ce17ac |
Code Coverage - Frontend unit tests
Test suite run success5440 tests passing in 701 suites. Report generated by 🧪jest coverage report action from 9ce17ac |
| getFilter(): BulkActionFilter; | ||
| changeState(): void; | ||
| getSocket(): Socket; | ||
| writeToReport(keyName: string, success: boolean, error?: string): void; |
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.
key name shouldn't be a string. We must use Buffers
| const errors = []; | ||
|
|
||
| res.forEach(([err], i) => { | ||
| const keyName = keys[i].toString(); |
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 must use Buffers
redisinsight/api/src/modules/bulk-actions/models/bulk-action-summary.spec.ts
Show resolved
Hide resolved
| @Get(':id/report/download') | ||
| async downloadReport( | ||
| @Param() { id }: BulkActionIdDto, | ||
| @Res() res: Response, | ||
| ): Promise<void> { | ||
| await this.service.streamReport(id, res); | ||
| } |
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.
This solution does not support multi-tenancy we need to rework it in the future
ArtemHoruzhenko
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 can go as is with assumption that we will back to it soon and enhance
redisinsight/ui/src/components/bulk-actions-config/BulkActionsConfig.tsx
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/components/bulk-actions-config/BulkActionsConfig.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private getDownloadUrl(): string { | ||
| return `databases/${this.databaseId}/bulk-actions/${this.id}/report/download`; |
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.
Bug: URL path mismatch prevents report downloads
The getDownloadUrl() method generates a URL with a databases/${databaseId}/ prefix, but the BulkActionsController is registered at just /bulk-actions without that prefix. The generated URL databases/{databaseId}/bulk-actions/{id}/report/download won't match the actual controller endpoint /bulk-actions/{id}/report/download, causing 404 errors when the frontend attempts to download reports.
What
Bulk delete can include a huge number of keys, and our current implementation keeps them in memory. So the robust solution in this case is to stream all the data to a report (successes and failures).
So these changes on a high level look like this:
generateReport: truedownloadUrlin the overview, but waits before processing (viawaitForStreamIfNeeded())/bulk-actions/:id/report(automatic, no button click)setStreamingResponse(res):bulkAction.writeToReport()=> writes directly to HTTP responsefinalizeReport()writes summary and callsres.end()Update
After the copilot suggestion - if the HTTP request that starts the actual delete process, for some reason, hangs, the user receives an error that the bulk delete didn't work. If they disable the report, the delete process starts immediately.
Screen.Recording.2025-12-01.at.14.21.11.mov
Testing
*)Expected result: All good
Actual result: it fails at some point, like in the following recording:
Screen.Recording.2025-11-27.at.15.33.02.mov
After
Screen.Recording.2025-12-01.at.13.31.40.mov
The result log format
Note
Adds streaming bulk delete reports with a new download endpoint, updates processing to write directly to the stream, and introduces a UI checkbox that auto-starts the download.
GET /bulk-actions/:id/report/downloadinbulk-actions.controller.tsandstreamReport(id, res)inBulkActionsServiceto set headers and attach the response stream.models/bulk-action.ts(waitForStreamIfNeeded,setStreamingResponse,writeToReport,finalizeReport, timeout handling) and includedownloadUrlanderroringetOverview.generateReport?: booleantoCreateBulkActionDto; pass through inBulkActionsProvider.create.bulkAction.writeToReport(...)while maintaining counters.IBulkActionOverviewwithdownloadUrlanderror; extendIBulkActionwithwriteToReport.bulkDelete.generateReport) to control report generation.generateReport; whendownloadUrlarrives, trigger download viatriggerDownloadFromUrlusinggetBaseUrl().error; remove legacy summary download button.triggerDownloadFromUrlhelper.Written by Cursor Bugbot for commit 9ce17ac. This will update automatically on new commits. Configure here.