feat: delete staging and load files post successful sync #5428
Conversation
shekhar-rudder
force-pushed
the
war-134-delete-files
branch
2 times, most recently
from
b07c7f6 to
f333dfc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5428 +/- ##
==========================================
- Coverage 74.82% 74.76% -0.07%
==========================================
Files 440 440
Lines 61668 61705 +37
==========================================
- Hits 46145 46134 -11
- Misses 12987 13021 +34
- Partials 2536 2550 +14 ☔ View full report in Codecov by Sentry. |
shekhar-rudder
force-pushed
the
war-134-delete-files
branch
from
f333dfc to
47e7504
Compare
shekhar-rudder
requested review from
am6010,
RanjeetMishra,
vyeshwanth and
achettyiitr
| if err = job.cleanupObjectStorageFiles(); err != nil { | ||
| break | ||
| } |
There was a problem hiding this comment.
@shekhar-rudder Say if this fails. Would the job be retried? If yes, in next retry would it continue from just cleanupObjectStorageFiles?
There was a problem hiding this comment.
@shekhar-rudder @achettyiitr Also from PRD doc, did we close on failing the job or considering metric and internal alert for now?
There was a problem hiding this comment.
@RanjeetMishra If the cleanup fails, the job will be marked as failed. On the next retry, I would assume it will proceed as it would for any other error.
There was a problem hiding this comment.
Since now it is config based deletion, I think its fine to mark the job as failed. If customer doesn't want the syncs to error out, they can always turn it off.
There was a problem hiding this comment.
Seems fine if we are aligning with marking sync jobs as failed. Regarding destination config for old and new destinations lets finalize tomorrow. This PR is good to go in that case.
warehouse/router/upload.go
Outdated
| filesToDelete := make([]string, len(job.stagingFiles)+len(loadingFiles)) | ||
| for i, file := range job.stagingFiles { | ||
| filesToDelete[i] = fm.GetDownloadKeyFromFileLocation(file.Location) | ||
| } | ||
| for i, file := range loadingFiles { | ||
| filesToDelete[i+len(job.stagingFiles)] = fm.GetDownloadKeyFromFileLocation(file.Location) | ||
| } |
There was a problem hiding this comment.
minor: Can see if you like lo map here.
fileKeysToDel := lo.Map(job.stagingFiles, func(file FileType, _ int) string {
return fm.GetDownloadKeyFromFileLocation(file.Location)
})
loadingFilesKeys := lo.Map(loadingFiles, func(file FileType, _ int) string {
return fm.GetDownloadKeyFromFileLocation(file.Location)
})
fileKeysToDel = append(fileKeysToDel, loadingFilesKeys...)
Description
This PR introduces support for cleaning up staging and load files from object storage based on the user preference (cleanupObjectStorageFiles). If file deletion fails, the job will be marked as failed. The cleanup process occurs just before marking the job as complete.
Implementation Details
The
UploadJobstruct already includesstagingFiles. Using their ids, the corresponding load files are retrieved. Keys are then computed from the locations of both file types, and the files are deleted.Security