-
-
Notifications
You must be signed in to change notification settings - Fork 700
Adding missing task run hierarchy to TaskRun table #1332
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
|
WalkthroughThe changes encompass enhancements across multiple components and services within the application, focusing on improving the handling and representation of task runs. Key modifications include the introduction of new methods and properties for data retrieval, restructuring response schemas to capture relationships among task runs, and the addition of new fields in the OpenAPI specification to support hierarchical task management. These updates enhance the overall functionality and clarity of task execution tracking. Changes
Possibly related PRs
Poem
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 (
|
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.
Actionable comments posted: 18
Outside diff range and nitpick comments (7)
references/v3-catalog/src/trigger/taskHierarchy.ts (2)
76-81
: Ensure consistent logging across all tasksWhile
greatGrandChildTask
logs"great-grand-child-task"
, consider verifying that all tasks log their execution consistently. This helps in maintaining uniformity and ease of debugging.Optionally, you might want to standardize the log messages or include additional context if needed.
1-2
: Organize imports for clarityCurrently, the imports combine both named imports and namespace imports. For better readability, consider grouping similar imports together.
- import { runs, task } from "@trigger.dev/sdk/v3"; - import { setTimeout } from "node:timers/promises"; + import { task } from "@trigger.dev/sdk/v3"; + import { runs } from "@trigger.dev/sdk/v3/runs"; + import { setTimeout } from "node:timers/promises";This separates your imports by functionality or source, enhancing code clarity.
apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (3)
215-215
: Handle potentialnull
orundefined
fortaskRun.depth
When adding
depth: taskRun.depth
, consider thattaskRun.depth
may benull
orundefined
. To prevent potential issues, you might want to provide a default value.Apply this change to provide a default value:
- depth: taskRun.depth, + depth: taskRun.depth ?? 0,
405-410
: Evaluate the necessity ofapiBooleanHelpersFromTaskRunStatus
methodThe method
apiBooleanHelpersFromTaskRunStatus
convertsTaskRunStatus
toRunStatus
and then to boolean helpers. Consider if this indirection is necessary, or if it would be more efficient to directly generate the boolean helpers fromTaskRunStatus
.
Line range hint
25-410
: Consider error handling for missingtaskRun
Currently, if
taskRun
is not found, the method returnsundefined
, and a debug log is recorded. Ensure that the calling function handles thisundefined
case gracefully, or consider throwing an error to make the failure more explicit.Optionally, you could throw an error:
if (!taskRun) { logger.debug("Task run not found", { friendlyId, envId: env.id }); - return undefined; + throw new Error(`TaskRun with friendlyId ${friendlyId} not found.`); }packages/database/prisma/schema.prisma (2)
1723-1723
: Typo in comment: Duplicate 'that'The comment on line 1723 contains a duplicated word "that":
"This represents the original task that that was triggered outside of a Trigger.dev task."
Please remove the extra "that" for clarity.
2107-2107
: Rename field 'TaskRun' to 'taskRuns' for consistencyIn the
BatchTaskRun
model, the field is currently namedTaskRun TaskRun[]
. To follow naming conventions and improve code readability, please rename it totaskRuns TaskRun[]
.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
Files selected for processing (16)
- apps/webapp/app/components/runs/v3/RunInspector.tsx (1 hunks)
- apps/webapp/app/db.server.ts (5 hunks)
- apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (5 hunks)
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts (1 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (1 hunks)
- apps/webapp/app/v3/services/batchTriggerTask.server.ts (1 hunks)
- apps/webapp/app/v3/services/triggerTask.server.ts (6 hunks)
- apps/webapp/package.json (1 hunks)
- docs/v3-openapi.yaml (2 hunks)
- packages/core/src/v3/schemas/api.ts (2 hunks)
- packages/database/prisma/migrations/20240918144036_add_task_run_parent_columns/migration.sql (1 hunks)
- packages/database/prisma/migrations/20240918150510_add_root_task_run_and_descendants/migration.sql (1 hunks)
- packages/database/prisma/migrations/20240919203806_add_depth_to_task_run/migration.sql (1 hunks)
- packages/database/prisma/schema.prisma (4 hunks)
- packages/trigger-sdk/src/v3/shared.ts (2 hunks)
- references/v3-catalog/src/trigger/taskHierarchy.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/webapp/package.json
Additional comments not posted (24)
packages/database/prisma/migrations/20240919203806_add_depth_to_task_run/migration.sql (1)
1-2
: LGTM! The SQL statement correctly adds thedepth
column to theTaskRun
table.The addition of the
depth
column aligns with the PR objective of introducing a hierarchy structure for task runs. Setting the column as NOT NULL with a default value of 0 ensures data integrity for existing task runs.A few additional suggestions:
- Ensure that any existing queries or application code that interacts with the
TaskRun
table are updated to handle the newdepth
column.- Consider adding database-level constraints or triggers to validate the integrity of the depth hierarchy, such as ensuring that a task run's depth is always greater than its parent's depth.
- Update the relevant documentation or data dictionary to reflect the new
depth
column and its purpose.packages/database/prisma/migrations/20240918150510_add_root_task_run_and_descendants/migration.sql (2)
1-2
: LGTM!The SQL statement correctly adds a new column named
rootTaskRunId
of typeTEXT
to theTaskRun
table. This allows establishing a hierarchical relationship among task runs.
4-5
: Looks good!The SQL statement correctly adds a foreign key constraint named
TaskRun_rootTaskRunId_fkey
to theTaskRun
table. The constraint references theid
column of the same table, establishing a self-referential relationship. TheON DELETE SET NULL
andON UPDATE CASCADE
clauses handle the cases of deleted or updated referenced task runs appropriately, ensuring data integrity.apps/webapp/app/v3/services/batchTriggerTask.server.ts (1)
116-116
: LGTM! The change enhances the clarity and control flow of batch processing.The new conditional assignment for the
parentBatch
property within theoptions
object improves the clarity and control flow of batch processing by explicitly distinguishing between scenarios where a batch is triggered independently versus when it is dependent on another attempt.
- When
dependentAttempt
is not present,parentBatch
is set tobatch.friendlyId
, indicating that the batch is triggered independently.- When
dependentAttempt
is present,parentBatch
is set toundefined
, indicating that the batch is dependent on another attempt.This change aligns with the PR objective of incorporating a hierarchy structure for task runs and provides more comprehensive data about task runs and their hierarchical relationships.
apps/webapp/app/db.server.ts (2)
1-1
: ImportingwithOptimize
from@prisma/extension-optimize
is a good addition.The
withOptimize
function is used to optimize the Prisma client by reducing the bundle size and improving the startup time. It does this by removing unused Prisma Client code and generating a minimal Prisma Client based on the used features.
Line range hint
98-119
: The modifications to the Prisma client logging configuration look good.The changes introduce a more flexible and granular logging setup:
- Additional logging levels, specifically for query events, are conditionally included based on the
VERBOSE_PRISMA_LOGS
environment variable. This allows for more detailed logging during development or debugging when needed.- Removing the commented-out sections related to logging streamlines the code and focuses on the new logging strategy.
These modifications enhance the functionality of the Prisma client while improving the clarity and control over logging behavior.
Also applies to: 149-170
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (1)
232-232
: LGTM!The addition of the
id
property to the return object is a valid enhancement. It provides a unique identifier for the run, which can be useful for tracking, referencing, or performing operations related to specific run instances. The value is correctly set torun.id
, maintaining consistency with the existing code.apps/webapp/app/components/runs/v3/RunInspector.tsx (1)
357-360
: LGTM!The new
Property.Item
correctly displays the "Run ID" and its value, enhancing the context provided by theRunInspector
component. The change is consistent with the existing code structure and conventions.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (1)
596-603
: LGTM!The additions of the "Run ID" and "Internal ID" properties to the
Property.Table
component within theRunBody
component look good. They provide more visibility into the run identifiers by displaying the values ofrun.friendlyId
andrun.id
respectively.packages/trigger-sdk/src/v3/shared.ts (2)
794-794
: LGTM!The change looks good. It allows passing the parent attempt ID when triggering a task, enabling tracking of the task run hierarchy. The
taskContext.ctx?.attempt.id
safely accesses the attempt ID from the current task context.
865-865
: Looks good!The change is consistent with the one made in
trigger_internal
. It allows passing the parent attempt ID when batch triggering tasks, enabling tracking of the task run hierarchy within a batch. ThetaskContext.ctx?.attempt.id
safely accesses the attempt ID from the current task context.packages/database/prisma/migrations/20240918144036_add_task_run_parent_columns/migration.sql (2)
2-5
: Ensure data type consistency between foreign keys and referenced keysThe new columns
parentTaskRunId
,parentTaskRunAttemptId
, andbatchId
are of typeTEXT
. Please confirm that theid
columns in theTaskRun
,TaskRunAttempt
, andBatchTaskRun
tables are also of typeTEXT
. Mismatched data types between foreign keys and primary keys can lead to constraint violations and runtime errors.To verify the data types of the referenced
id
columns, you can run:#!/bin/bash # Description: Verify that the data types of foreign keys match the primary keys. # Retrieve schema definitions for the referenced tables. rg --type sql 'CREATE TABLE "?(TaskRun|TaskRunAttempt|BatchTaskRun)"?' -A 20This will display the table definitions, allowing you to check the data types of the
id
columns.
11-11
: Verify thatON DELETE SET NULL
is appropriate for foreign key constraintsThe foreign key constraints for
parentTaskRunId
,parentTaskRunAttemptId
, andbatchId
are set toON DELETE SET NULL
. This means that when a referenced record is deleted, the corresponding foreign key inTaskRun
will be set toNULL
. Please verify that this behavior aligns with the application's data integrity requirements. Depending on the business logic, you might consider usingON DELETE CASCADE
to delete dependentTaskRun
records automatically, orON DELETE RESTRICT
to prevent deletion of referenced records.To examine how deletions of referenced records are handled in the codebase, you can run the following script:
This will help identify any delete operations on the referenced tables and allow you to assess whether
ON DELETE SET NULL
is suitable.Also applies to: 14-14, 17-17
Verification successful
ON DELETE SET NULL is appropriate for these foreign key constraints
The current schema design using ON DELETE SET NULL for parentTaskRunId, parentTaskRunAttemptId, and batchId in the TaskRun model is appropriate. This approach allows TaskRun records to persist even when related parent runs, attempts, or batches are deleted, maintaining data integrity while allowing flexibility in relationship management.
Key points:
- TaskRun relationships with parent runs and batches use ON DELETE SET NULL, preserving the TaskRun record if parents are deleted.
- Other relationships in TaskRun, TaskRunAttempt, and BatchTaskRun models use ON DELETE CASCADE where appropriate, ensuring proper cleanup of directly dependent records.
- The absence of custom deletion logic suggests reliance on database-level cascading and nullification, which is a clean and efficient approach.
This design balances data integrity with the need for flexible relationship management in a complex task execution system.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for DELETE operations on referenced tables and assess implications. # Find where deletions of TaskRun are performed. rg --type sql 'DELETE\s+FROM\s+"?TaskRun"?' -A 3 # Find where deletions of TaskRunAttempt are performed. rg --type sql 'DELETE\s+FROM\s+"?TaskRunAttempt"?' -A 3 # Find where deletions of BatchTaskRun are performed. rg --type sql 'DELETE\s+FROM\s+"?BatchTaskRun"?' -A 3Length of output: 17019
Script:
#!/bin/bash # Search for Prisma schema definitions and application code related to deletions # Search for Prisma schema definitions echo "Prisma schema definitions:" rg --type ts 'model\s+(TaskRun|TaskRunAttempt|BatchTaskRun)' -A 10 # Search for deletion-related code in TypeScript files echo "\nDeletion-related code:" rg --type ts '(delete|remove)(TaskRun|TaskRunAttempt|BatchTaskRun)' -A 5 # Search for Prisma client delete operations echo "\nPrisma client delete operations:" rg --type ts 'prisma.*delete.*where.*TaskRun|TaskRunAttempt|BatchTaskRun' -A 5Length of output: 45619
Script:
#!/bin/bash # Search for specific foreign key constraints and their handling # Look for schema definitions of TaskRun, TaskRunAttempt, and BatchTaskRun echo "Schema definitions:" rg --type ts 'model\s+(TaskRun|TaskRunAttempt|BatchTaskRun)' -A 20 # Search for code handling parentTaskRunId, parentTaskRunAttemptId, or batchId echo "\nHandling of foreign key fields:" rg --type ts '(parentTaskRunId|parentTaskRunAttemptId|batchId)' -A 5 # Look for implementation of CancelTaskRunService echo "\nCancelTaskRunService implementation:" rg --type ts 'class CancelTaskRunService' -A 20 # Search for any code that might be nullifying these foreign keys echo "\nPossible nullification of foreign keys:" rg --type ts '(parentTaskRunId|parentTaskRunAttemptId|batchId)\s*=\s*null' -A 5Length of output: 15809
Script:
#!/bin/bash # Search for schema definitions and deletion handling # Look for Prisma schema definitions echo "Prisma schema definitions:" rg --type ts '(model\s+(TaskRun|TaskRunAttempt|BatchTaskRun)|@relation)' -A 10 -g 'schema.prisma' # Search for any DELETE operations in SQL migrations echo "\nDELETE operations in migrations:" rg --type sql 'DELETE\s+FROM' -A 5 # Look for any code that might be handling deletions of related records echo "\nDeletion handling code:" rg --type ts '(delete|remove)(TaskRun|TaskRunAttempt|BatchTaskRun)' -A 10Length of output: 97723
apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (3)
25-28
: Confirm the use offindFirst
instead offindUnique
The query now uses
findFirst
to retrieve thetaskRun
. Ensure that this change is intentional and that handling multiple matching records by returning the first one is acceptable for your use case.
39-66
: Verify the inclusion of related task runs in the queryThe addition of
parentTaskRun
,rootTaskRun
, andchildRuns
in theinclude
clause enhances the data retrieved. Confirm that all necessary fields are selected and that this does not introduce any performance issues due to the increased data retrieval.
93-126
: Ensure all required fields are selected inchildRuns
The fields selected for
childRuns
seem comprehensive. Double-check that all necessary fields for your application's requirements are included, and remove any unnecessary fields to optimize the query performance.packages/core/src/v3/schemas/api.ts (3)
71-73
: Addition ofparentAttempt
andparentBatch
fields approvedThe addition of optional
parentAttempt
andparentBatch
fields toTriggerTaskRequestBody
enhances the ability to track task dependencies and hierarchy. The implementation is consistent with existing patterns and properly typed.
499-499
: Addition ofdepth
field toCommonRunFields
Including the
depth
field inCommonRunFields
allows for tracking the hierarchical level of each run. This enhancement aligns well with the PR objectives to represent task run hierarchies.
524-528
: Addition ofrelatedRuns
field enhances run hierarchy representationThe new
relatedRuns
field inRetrieveRunResponse
, which includesroot
,parent
, andchildren
, effectively structures the relationships among runs. This addition aligns with the PR objectives and improves the clarity of the response data.apps/webapp/app/v3/services/triggerTask.server.ts (2)
111-112
: LGTMIncluding
rootTaskRunId
anddepth
in the select clause ensures that the complete hierarchy information of the task run is retrieved.
167-168
: LGTMAdding
rootTaskRunId
anddepth
to the select clause fordependentBatchRun
aligns with the retrieval of necessary hierarchy details.packages/database/prisma/schema.prisma (2)
2105-2106
: Addition of 'createdAt' and 'updatedAt' fieldsIncluding
createdAt
andupdatedAt
timestamps in theBatchTaskRun
model is beneficial for tracking record creation and modification times.
1914-1914
: Child runs relation in 'TaskRunAttempt' established correctlyThe addition of
childRuns
in theTaskRunAttempt
model correctly establishes the reverse relation toparentTaskRunAttempt
inTaskRun
, enabling navigation between parent attempts and their child runs.docs/v3-openapi.yaml (1)
Line range hint
1824-1912
: Refactoring withCommonRunObject
enhances schema consistencyThe introduction of
CommonRunObject
consolidates shared properties of run objects, improving reusability and maintainability across the API specification.
ALTER TABLE "TaskRun" ADD COLUMN "batchId" TEXT, | ||
ADD COLUMN "parentTaskRunAttemptId" TEXT, | ||
ADD COLUMN "parentTaskRunId" TEXT, | ||
ADD COLUMN "resumeParentOnCompletion" BOOLEAN NOT NULL DEFAULT false; |
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.
Consider adding indexes on all foreign key columns for improved performance
An index has been created on parentTaskRunId
, which will optimize queries filtering on this column. To further enhance query performance, especially for joins and lookups involving foreign keys, consider adding indexes on the other foreign key columns: parentTaskRunAttemptId
and batchId
.
You can add the following indexes:
CREATE INDEX "TaskRun_parentTaskRunAttemptId_idx" ON "TaskRun"("parentTaskRunAttemptId");
CREATE INDEX "TaskRun_batchId_idx" ON "TaskRun"("batchId");
export const rootTask = task({ | ||
id: "task-hierarchy/root-task", | ||
run: async ( | ||
{ useWaits = true, useBatch = false }: { useWaits: boolean; useBatch: boolean }, | ||
{ ctx } | ||
) => { | ||
console.log("root-task"); | ||
|
||
if (useWaits) { | ||
if (useBatch) { | ||
await childTask.batchTriggerAndWait([{ payload: { useWaits, useBatch } }]); | ||
} else { | ||
await childTask.triggerAndWait({ useWaits, useBatch }); | ||
} | ||
} else { | ||
if (useBatch) { | ||
await childTask.batchTrigger([{ payload: { useWaits, useBatch } }]); | ||
} else { | ||
await childTask.trigger({ useWaits, useBatch }); | ||
} | ||
} | ||
|
||
if (!useWaits) { | ||
await setTimeout(10_000); // Wait for 10 seconds, all the runs will be finished by then | ||
} | ||
|
||
await logRunHierarchy(ctx.run.id); | ||
}, | ||
}); |
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.
Refactor repetitive task triggering logic to improve maintainability
The rootTask
, childTask
, and grandChildTask
have similar code blocks for triggering child tasks based on the useWaits
and useBatch
flags. This repetition can make the code harder to maintain and increases the risk of inconsistencies or bugs.
Consider extracting the task triggering logic into a helper function. Here's how you might implement it:
async function triggerTask(
taskFunction: any,
useWaits: boolean,
useBatch: boolean,
payload: { useWaits: boolean; useBatch: boolean }
) {
if (useWaits) {
if (useBatch) {
await taskFunction.batchTriggerAndWait([{ payload }]);
} else {
await taskFunction.triggerAndWait(payload);
}
} else {
if (useBatch) {
await taskFunction.batchTrigger([{ payload }]);
} else {
await taskFunction.trigger(payload);
}
}
}
Then, you can simplify the run
functions:
export const rootTask = task({
id: "task-hierarchy/root-task",
run: async (
{ useWaits = true, useBatch = false }: { useWaits: boolean; useBatch: boolean },
{ ctx }
) => {
console.log("root-task");
- if (useWaits) {
- if (useBatch) {
- await childTask.batchTriggerAndWait([{ payload: { useWaits, useBatch } }]);
- } else {
- await childTask.triggerAndWait({ useWaits, useBatch });
- }
- } else {
- if (useBatch) {
- await childTask.batchTrigger([{ payload: { useWaits, useBatch } }]);
- } else {
- await childTask.trigger({ useWaits, useBatch });
- }
- }
+ await triggerTask(childTask, useWaits, useBatch, { useWaits, useBatch });
if (!useWaits) {
await setTimeout(10_000); // Wait for 10 seconds, all the runs will be finished by then
}
await logRunHierarchy(ctx.run.id);
},
});
Apply similar changes to childTask
and grandChildTask
to reduce duplication across your tasks.
await setTimeout(10_000); // Wait for 10 seconds, all the runs will be finished by then | ||
} |
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.
Replace fixed timeout with proper task completion handling
Using a fixed delay with setTimeout(10_000)
assumes that all child tasks will complete within 10 seconds. This approach can lead to unreliable behavior if tasks take longer than expected or if they're faster, unnecessarily delaying execution.
Consider implementing a mechanism to confirm that all child tasks have completed before proceeding. Since you're already leveraging task dependencies, you might:
- Use
triggerAndWait
for non-batch tasks even whenuseWaits
is false, if appropriate. - Implement a status check loop to verify completion:
if (!useWaits) {
let isComplete = false;
while (!isComplete) {
const runStatus = await runs.getStatusOfChildTasks(ctx.run.id);
isComplete = runStatus.every((status) => status.isCompleted);
if (!isComplete) {
await setTimeout(1000); // Poll every second
}
}
}
This ensures that you proceed only after confirming all child tasks have finished.
runId: string, | ||
parentTaskIdentifier?: string, | ||
triggerFunction?: string | ||
) { | ||
const runData = await runs.retrieve(runId); | ||
|
||
const indent = " ".repeat(runData.depth * 2); | ||
const triggerInfo = triggerFunction ? ` (triggered by ${triggerFunction})` : ""; | ||
const parentInfo = parentTaskIdentifier ? ` (parent task: ${parentTaskIdentifier})` : ""; | ||
|
||
console.log( | ||
`${indent}Level ${runData.depth}: [${runData.taskIdentifier}] run ${runData.id}${triggerInfo}${parentInfo}` | ||
); | ||
|
||
for (const childRun of runData.relatedRuns.children ?? []) { | ||
await logRunHierarchy(childRun.id, runData.taskIdentifier, childRun.triggerFunction); | ||
} | ||
} |
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.
Add null-check for relatedRuns
to prevent potential errors
In the logRunHierarchy
function, runData.relatedRuns
may be undefined
if there are no related runs. Accessing runData.relatedRuns.children
without checking can lead to runtime errors.
Modify your code to safely handle the absence of relatedRuns
:
- for (const childRun of runData.relatedRuns.children ?? []) {
+ const children = runData.relatedRuns?.children ?? [];
+ for (const childRun of children) {
await logRunHierarchy(childRun.id, runData.taskIdentifier, childRun.triggerFunction);
}
This ensures that the function doesn't throw an error when there are no child runs.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
runId: string, | |
parentTaskIdentifier?: string, | |
triggerFunction?: string | |
) { | |
const runData = await runs.retrieve(runId); | |
const indent = " ".repeat(runData.depth * 2); | |
const triggerInfo = triggerFunction ? ` (triggered by ${triggerFunction})` : ""; | |
const parentInfo = parentTaskIdentifier ? ` (parent task: ${parentTaskIdentifier})` : ""; | |
console.log( | |
`${indent}Level ${runData.depth}: [${runData.taskIdentifier}] run ${runData.id}${triggerInfo}${parentInfo}` | |
); | |
for (const childRun of runData.relatedRuns.children ?? []) { | |
await logRunHierarchy(childRun.id, runData.taskIdentifier, childRun.triggerFunction); | |
} | |
} | |
runId: string, | |
parentTaskIdentifier?: string, | |
triggerFunction?: string | |
) { | |
const runData = await runs.retrieve(runId); | |
const indent = " ".repeat(runData.depth * 2); | |
const triggerInfo = triggerFunction ? ` (triggered by ${triggerFunction})` : ""; | |
const parentInfo = parentTaskIdentifier ? ` (parent task: ${parentTaskIdentifier})` : ""; | |
console.log( | |
`${indent}Level ${runData.depth}: [${runData.taskIdentifier}] run ${runData.id}${triggerInfo}${parentInfo}` | |
); | |
const children = runData.relatedRuns?.children ?? []; | |
for (const childRun of children) { | |
await logRunHierarchy(childRun.id, runData.taskIdentifier, childRun.triggerFunction); | |
} | |
} |
finishedAt: taskRun.parentTaskRun.completedAt ?? undefined, | ||
expiredAt: taskRun.parentTaskRun.expiredAt ?? undefined, | ||
delayedUntil: taskRun.parentTaskRun.delayUntil ?? undefined, | ||
isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id, |
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.
Safely compare IDs in isRoot
property
The expression taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id
may cause issues if taskRun.rootTaskRun
is undefined
. Consider adding a null check to prevent potential runtime errors.
Apply this change for a safer comparison:
- isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id,
+ isRoot: taskRun.rootTaskRun
+ ? taskRun.parentTaskRun.id === taskRun.rootTaskRun.id
+ : false,
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id, | |
isRoot: taskRun.rootTaskRun | |
? taskRun.parentTaskRun.id === taskRun.rootTaskRun.id | |
: false, |
rootTaskRun TaskRun? @relation("TaskRootRun", fields: [rootTaskRunId], references: [id], onDelete: SetNull) | ||
rootTaskRunId String? |
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.
Consider adding an index on 'rootTaskRunId'
To optimize queries involving the rootTaskRunId
field, consider adding an index on this field, similar to the index on parentTaskRunId
. This can improve query performance when retrieving root task runs.
parentTaskRunAttempt TaskRunAttempt? @relation("TaskParentRunAttempt", fields: [parentTaskRunAttemptId], references: [id], onDelete: SetNull) | ||
parentTaskRunAttemptId String? |
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.
Consider adding an index on 'parentTaskRunAttemptId'
Adding an index on the parentTaskRunAttemptId
field can enhance the performance of queries that involve parent task run attempts. This is especially useful if you frequently query or join on this field.
batch BatchTaskRun? @relation(fields: [batchId], references: [id], onDelete: SetNull) | ||
batchId String? |
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.
Consider adding an index on 'batchId'
For improved query efficiency when accessing batch-related data, consider adding an index on the batchId
field in the TaskRun
model.
required: | ||
- id | ||
- generator | ||
- attempts |
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.
Redundant attempts
in required
properties
The attempts
property is already declared as required in CommonRunObject
. Including it again in RetrieveRunResponse
is redundant and can be removed.
Apply this diff to remove the redundant attempts
requirement:
required:
- - attempts
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- attempts | |
required: |
docs/v3-openapi.yaml
Outdated
depth: | ||
type: number | ||
example: 0 | ||
description: The depth of the run in the task run hierarchy. The root run has a depth of 0. |
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.
Change depth
property type to integer
The depth
property represents hierarchical levels and should be defined as type integer
rather than number
for accurate data modeling.
Apply the following diff to correct the type:
depth:
- type: number
+ type: integer
example: 0
description: The depth of the run in the task run hierarchy. The root run has a depth of 0.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
depth: | |
type: number | |
example: 0 | |
description: The depth of the run in the task run hierarchy. The root run has a depth of 0. | |
depth: | |
type: integer | |
example: 0 | |
description: The depth of the run in the task run hierarchy. The root run has a depth of 0. |
commit: |
3ba198b
to
0b984a5
Compare
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/v3/schemas/api.ts (3 hunks)
Additional comments not posted (5)
packages/core/src/v3/schemas/api.ts (5)
71-71
: LGTM!The addition of the optional
parentAttempt
field to theTriggerTaskRequestBody
schema is a valid change. It allows specifying the parent attempt ID when triggering a task, enabling the establishment of parent-child relationships between task attempts.
73-73
: LGTM!The addition of the optional
parentBatch
field to theTriggerTaskRequestBody
schema is a valid change. It allows specifying the parent batch ID when triggering a task, enabling the establishment of parent-child relationships between task batches.
475-482
: LGTM!The introduction of the
TriggerFunction
enum is a valuable addition. It provides a clear specification of the types of trigger functions that can be utilized, covering both single task triggers and batch triggers. The enum improves code readability and maintainability by providing a predefined set of valid trigger function types.
510-515
: LGTM!The
RelatedRunDetails
schema is a well-structured representation of related run details. It inherits the common fields fromCommonRunFields
using the spread operator, ensuring consistency with other run-related schemas. The addition of thedepth
field allows specifying the depth of the related run in the hierarchy, while thetriggerFunction
field utilizes the previously definedTriggerFunction
enum to ensure valid trigger function types are used. The optionalbatchId
field provides the flexibility to associate the related run with a specific batch.
524-528
: LGTM!The addition of the
relatedRuns
field to theRetrieveRunResponse
schema is a valuable enhancement. It allows representing the relationships among runs by providing fields for the root run, parent run, and an array of child runs. The use of theRelatedRunDetails
type ensures consistency with the previously defined schema for related run details. This structured approach improves the clarity and usability of the response data, facilitating better management of task execution flows.
export const RelatedRunDetails = z.object({ | ||
...CommonRunFields, | ||
depth: z.number(), | ||
triggerFunction: z.enum(["triggerAndWait", "trigger", "batchTriggerAndWait", "batchTrigger"]), |
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.
Use the TriggerFunction
enum type for the triggerFunction
field.
The triggerFunction
field in the RelatedRunDetails
schema should utilize the previously defined TriggerFunction
enum type instead of directly specifying the enum values.
Apply this diff to fix the field type:
-triggerFunction: z.enum(["triggerAndWait", "trigger", "batchTriggerAndWait", "batchTrigger"]),
+triggerFunction: TriggerFunction,
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
triggerFunction: z.enum(["triggerAndWait", "trigger", "batchTriggerAndWait", "batchTrigger"]), | |
triggerFunction: TriggerFunction, |
TaskRuns now keep track of their parent task run, their root task run, the depth in the hierarchy, whether they will be resuming their parent on completion (e.g. using triggerAndWait), and what batch they are apart of (we already had this but now it's denormalized). This allows us to add additional output to the
runs.retrieve()
endpoint:In the future we will use this data to:
Summary by CodeRabbit
Release Notes
New Features
depth
,parentBatch
, andrelatedRuns
for better task management.Documentation