Fix updating many environment variables at once #1413
Conversation
|
WalkthroughThe changes in this pull request focus on the Changes
Possibly related issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
Line range hint
138-199: Approve: Transaction logic restructuring improves performanceThe restructuring of the transaction logic addresses the issue of potential timeouts during bulk operations. By processing each variable in its own transaction, the code reduces the likelihood of timeouts and improves overall performance when handling a large number of environment variables.
Consider the following improvements:
- Add error handling for individual variable processing:
for (const variable of values) { + try { const result = await $transaction(this.prismaClient, async (tx) => { // ... (existing transaction logic) }); + } catch (error) { + console.error(`Error processing variable ${variable.key}:`, error); + // Decide whether to continue processing other variables or throw an error + } }
- For further optimization, consider implementing a batch processing approach:
const batchSize = 50; // Adjust based on performance testing for (let i = 0; i < values.length; i += batchSize) { const batch = values.slice(i, i + batchSize); await Promise.all(batch.map(async (variable) => { try { await $transaction(this.prismaClient, async (tx) => { // ... (existing transaction logic) }); } catch (error) { console.error(`Error processing variable ${variable.key}:`, error); } })); }This approach would process variables in parallel batches, potentially further improving performance while still maintaining reasonable transaction sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
Line range hint
1-799: Overall assessment: Changes address PR objectives effectivelyThe modifications to the
createmethod in theEnvironmentVariablesRepositoryclass successfully address the issue of potential transaction timeouts during bulk operations with environment variables. By processing each variable in its own transaction, the code reduces the likelihood of timeouts and improves overall performance when handling a large number of environment variables.The changes align well with the PR objectives and should resolve the reported issue. Consider implementing the suggested error handling and batch processing optimizations to further improve the robustness and efficiency of the code.
|
hey @ericallam @matt-aitken please review this PR |
commit: |
There was a problem hiding this comment.
Thanks @yadavshubham01! No biggie, but don't forget to lint next time.
nicktrn
changed the title
* refactor finalize run service * refactor complete attempt service * remove separate graceful exit handling * refactor task status helpers * clearly separate statuses in prisma schema * all non-final statuses should be failable * new import payload error code * store default retry config if none set on task * failed run service now respects retries * fix merged task retry config indexing * some errors should never be retried * finalize run service takes care of acks now * execution payload helper now with single object arg * internal error code enum export * unify failed and crashed run retries * Prevent uncaught socket ack exceptions (#1415) * catch all the remaining socket acks that could possibly throw * wrap the remaining handlers in try catch * New onboarding question (#1404) * Updated “Twitter” to be “X (Twitter)” * added Textarea to storybook * Updated textarea styling to match input field * WIP adding new text field to org creation page * Added description to field * Submit feedback to Plain when an org signs up * Formatting improvement * type improvement * removed userId * Moved submitting to Plain into its own file * Change orgName with name * use sendToPlain function for the help & feedback email form * use name not orgName * import cleanup * Downgrading plan form uses sendToPlain * Get the userId from requireUser only * Added whitespace-pre-wrap to the message property on the run page * use requireUserId * Removed old Plain submit code * Added a new Context page for the docs (#1416) * Added a new context page with task context properties * Removed code comments * Added more crosslinks * Fix updating many environment variables at once (#1413) * Move code example to the side menu * New docs example for creating a HN email summary * doc: add instructions to create new reference project and run it locally (#1417) * doc: add instructions to create new reference project and run it locally * doc: Add instruction for running tunnel * minor language improvement * Fix several restore and resume bugs (#1418) * try to correct resume messages with missing checkpoint * prevent creating checkpoints for outdated task waits * prevent creating checkpoints for outdated batch waits * use heartbeats to check for and clean up any leftover containers * lint * improve exec logging * improve resume attempt logs * fix for resuming parents of canceled child runs * separate SIGTERM from maybe OOM errors * pretty errors can have magic dashboard links * prevent uncancellable checkpoints * simplify task run error code enum export * grab the last, not the first child run * Revert "prevent creating checkpoints for outdated batch waits" This reverts commit f2b5c2a. * Revert "grab the last, not the first child run" This reverts commit 89ec5c8. * Revert "prevent creating checkpoints for outdated task waits" This reverts commit 11066b4. * more logs for resume message handling * add magic error link comment * add changeset * chore: Update version for release (#1410) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Release 3.0.13 * capture ffmpeg oom errors * respect maxAttempts=1 when failing before first attempt creation * request worker exit on fatal errors * fix error code merge * add new error code to should retry * pretty segfault errors * pretty internal errors for attempt spans * decrease oom false positives * fix timeline event color for failed runs * auto-retry packet import and export * add sdk version check and complete event while completing attempt * all internal errors become crashes by default * use pretty error helpers exclusively * error to debug log * zodfetch fixes * rename import payload to task input error * fix true non-zero exit error display * fix retry config parsing * correctly mark crashes as crashed * add changeset * remove non-zero exit comment * pretend we don't support default default retry configs yet --------- Co-authored-by: James Ritchie <james@trigger.dev> Co-authored-by: shubham yadav <126192924+yadavshubham01@users.noreply.github.com> Co-authored-by: Tarun Pratap Singh <101409098+Wackyator@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closes #
✅ Checklist
##ChangeLog
When bulk upserting environment variables, wrapping the entire operation in a single database transaction can lead to transaction timeouts if there are a large number of environment variables (e.g., more than 100). To solve this error, I can refactor the code by moving the transaction inside the for loop "for" that iterates over the environment variables. Inside the transaction, perform the necessary operations for each environment variable, such as upserting the variable, creating secret references, and setting secret values. Since each environment variable is now processed within its own transaction, errors will be isolated to individual variables
💯
Summary by CodeRabbit