Conversation
… methods for batch processing
There was a problem hiding this comment.
Pull request overview
This pull request refactors the AutoRemovePlugin by renaming configuration options for clarity, improving error messages, changing property visibility, and modifying the deletion batching logic. The PR title "Next" suggests this is part of an iterative development process.
Changes:
- Renamed
maxItemstokeepAtLeastandmaxAgetodeleteOlderThanfor better semantic clarity - Added new
minItemsKeepoption (though it's not implemented in the cleanup logic) - Removed
maxDeletePerRunoption and replaced with hardcoded batching of 100 items - Changed property visibility from protected/private to public for
resourceandtimer - Improved error messages to include resource labels and field names
- Fixed spelling errors in comments (режиму → mode, otem → item)
- Modified deletion to use Promise.all for batched parallel deletions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| types.ts | Renamed configuration options (maxItems → keepAtLeast, maxAge → deleteOlderThan), added minItemsKeep field, removed maxDeletePerRun field, fixed spelling errors in comments |
| index.ts | Updated to use renamed options, changed property visibility, improved error messages, refactored deletion logic to use Promise.all batching, removed MAX_DELETE_PER_RUN constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (let i = 0; i < toDelete.length; i += ITEMS_PER_DELETE) { | ||
| const deletePackage = toDelete.slice(i, i + ITEMS_PER_DELETE); | ||
| await Promise.all(deletePackage.map(r => resource.delete(r[pkColumn]))); |
There was a problem hiding this comment.
Batch deletion uses Promise.all() to delete 100 items in parallel, which could overwhelm the database with concurrent delete operations. Consider using sequential deletion or limiting concurrency (e.g., using a concurrency limiter pattern) to avoid potential database connection pool exhaustion or deadlocks, especially when deleting large numbers of records.
| const maxAgeMs = parseDuration(this.options.deleteOlderThan!); | ||
| const threshold = Date.now() - maxAgeMs; | ||
| const resource = adminforth.resource(this._resourceConfig.resourceId); | ||
| const resource = adminforth.resource(this.resource.resourceId); |
There was a problem hiding this comment.
The property this.resource might be undefined when cleanupByTime is called. Access to this.resource.resourceId and this.resource.columns on lines 91 and 96 could throw runtime errors if the resource is undefined.
2 participants
No description provided.