Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Complete Exports in Batches #2949

Merged
merged 8 commits into from Feb 10, 2022
Merged

Complete Exports in Batches #2949

merged 8 commits into from Feb 10, 2022

Conversation

evantahler
Copy link
Member

@evantahler evantahler commented Feb 8, 2022

This PR replaces the single export.update for a class method ExportOps.complete() accepting an array of Exports. This will speed up writing updates to successful exports.

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

@evantahler evantahler added the enhancement New feature or request label Feb 8, 2022
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's some work ongoing with tests.

I did have one question RE: the implementation in the sendExports() method.

core/src/modules/ops/destination.ts Outdated Show resolved Hide resolved
for (const _export of _exports) {
_export.state = "complete";
_export.completedAt = now;
await Export.logExport(_export);
Copy link
Contributor

@teallarson teallarson Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this doing the same thing twice? logExport is part of an @AfterUpdate on the Exports model.

Copy link
Member Author

@evantahler evantahler Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AfterUpdate only fires after a single-item update. There's @AfterBatchUpdate which happens after these kind of bulk updates, but they don't actually include the rows modified (because they would often be too big to fit in RAM). So, this kind of hack is needed

@evantahler evantahler merged commit f5c418d into main Feb 10, 2022
@evantahler evantahler deleted the update-exports-in-batch branch February 10, 2022 03:10
@evantahler evantahler linked an issue Feb 28, 2022 that may be closed by this pull request
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Performance Improvements
2 participants