[flyteadmin] add delete execution phase api #6267
Conversation
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #50b8feActionable Suggestions - 8
Additional Suggestions - 5
Review Details
AI Code Review powered by |
taieeuu
changed the title
taieeuu
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6267 +/- ##
==========================================
- Coverage 58.48% 58.47% -0.01%
==========================================
Files 937 937
Lines 71088 71138 +50
==========================================
+ Hits 41577 41601 +24
- Misses 26359 26382 +23
- Partials 3152 3155 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changelist by BitoThis pull request implements the following key changes.
|
| sizeCache protoimpl.SizeCache | ||
| unknownFields protoimpl.UnknownFields | ||
|
|
||
| ExecutionPhase string `protobuf:"bytes,1,opt,name=execution_phase,json=executionPhase,proto3" json:"execution_phase,omitempty"` |
There was a problem hiding this comment.
Consider adding validation for the execution_phase field in ExecutionPhaseDeleteRequest. The field appears to be used for deleting execution phases but lacks input validation to ensure valid phase values are provided. This could potentially lead to runtime errors if invalid phases are specified.
Code suggestion
Check the AI-generated fix before applying
| ExecutionPhase string `protobuf:"bytes,1,opt,name=execution_phase,json=executionPhase,proto3" json:"execution_phase,omitempty"` | |
| // +required The phase of execution to be deleted. Must be one of the valid execution phases. | |
| ExecutionPhase string `protobuf:"bytes,1,opt,name=execution_phase,json=executionPhase,proto3" json:"execution_phase,omitempty" validate:"required,oneof=QUEUED RUNNING SUCCEEDED FAILED ABORTED"` |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| m.Metrics.executionEndpointMetrics.terminate.Time(func() { | ||
| response, err = m.ExecutionManager.DeleteExecutionPhase(ctx, request) |
There was a problem hiding this comment.
Consider using a more specific metric name for DeleteExecutionPhase. Currently using terminate metric which seems to be shared with termination functionality. This could lead to misleading metrics.
Code suggestion
Check the AI-generated fix before applying
terminate util.RequestMetrics
+ deletePhase util.RequestMetrics
@@ -160,9 +160,9 @@
- m.Metrics.executionEndpointMetrics.terminate.Time(func() {
- response, err = m.ExecutionManager.DeleteExecutionPhase(ctx, request)
- })
- if err != nil {
- return nil, util.TransformAndRecordError(err, &m.Metrics.executionEndpointMetrics.terminate)
- }
- m.Metrics.executionEndpointMetrics.terminate.Success()
+ m.Metrics.executionEndpointMetrics.deletePhase.Time(func() {
+ response, err = m.ExecutionManager.DeleteExecutionPhase(ctx, request)
+ })
+ if err != nil {
+ return nil, util.TransformAndRecordError(err, &m.Metrics.executionEndpointMetrics.deletePhase)
+ }
+ m.Metrics.executionEndpointMetrics.deletePhase.Success()
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| result := r.db.Delete(&models.Execution{}, "phase = ?", executionPhase) | ||
| if result.Error != nil { | ||
| return result.Error | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Consider adding validation to ensure executionPhase is not empty and matches valid execution phase values. Also, consider adding error handling for when no records are deleted.
Code suggestion
Check the AI-generated fix before applying
| result := r.db.Delete(&models.Execution{}, "phase = ?", executionPhase) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| return nil | |
| if executionPhase == "" { | |
| return fmt.Errorf("execution phase cannot be empty") | |
| } | |
| result := r.db.Delete(&models.Execution{}, "phase = ?", executionPhase) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| if result.RowsAffected == 0 { | |
| return fmt.Errorf("no executions found with phase %s", executionPhase) | |
| } | |
| return nil |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| _va := make([]interface{}, len(opts)) | ||
| for _i := range opts { | ||
| _va[_i] = opts[_i] | ||
| } |
There was a problem hiding this comment.
Consider adding validation for the opts parameter in the DeleteExecutionPhase method. Currently, there's no check if opts is nil before iterating over it.
Code suggestion
Check the AI-generated fix before applying
| _va := make([]interface{}, len(opts)) | |
| for _i := range opts { | |
| _va[_i] = opts[_i] | |
| } | |
| _va := []interface{}{} | |
| if opts != nil { | |
| _va = make([]interface{}, len(opts)) | |
| for _i := range opts { | |
| _va[_i] = opts[_i] | |
| } | |
| } |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| get: "/api/v1/delete_execution_phase/{execution_phase}" | ||
| }; |
There was a problem hiding this comment.
The DeleteExecutionPhase endpoint is using GET HTTP method for a deletion operation. Consider using DELETE HTTP method instead to better align with REST conventions and make the API more intuitive.
Code suggestion
Check the AI-generated fix before applying
| get: "/api/v1/delete_execution_phase/{execution_phase}" | |
| }; | |
| delete: "/api/v1/delete_execution_phase/{execution_phase}" | |
| }; |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| err := m.db.ExecutionRepo().Delete(ctx, executionPhase) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Consider adding error wrapping with context when returning the database error. This would help with debugging by providing more context about what operation failed. Maybe something like: fmt.Errorf("failed to delete execution phase %s: %w", executionPhase, err)
Code suggestion
Check the AI-generated fix before applying
| return nil, err | |
| return nil, fmt.Errorf("failed to delete execution phase %s: %w", executionPhase, err) |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| protoReq.ExecutionPhase, err = runtime.String(val) | ||
| if err != nil { | ||
| return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "execution_phase", err) | ||
| } |
There was a problem hiding this comment.
Consider adding validation for empty string in execution_phase parameter. The current implementation accepts empty strings which could lead to unintended behavior when deleting execution phases.
Code suggestion
Check the AI-generated fix before applying
| protoReq.ExecutionPhase, err = runtime.String(val) | |
| if err != nil { | |
| return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "execution_phase", err) | |
| } | |
| protoReq.ExecutionPhase, err = runtime.String(val) | |
| if err != nil { | |
| return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "execution_phase", err) | |
| } | |
| if protoReq.ExecutionPhase == "" { | |
| return nil, metadata, status.Errorf(codes.InvalidArgument, "parameter %s cannot be empty", "execution_phase") | |
| } |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| in := new(admin.ExecutionPhaseDeleteRequest) | ||
| if err := dec(in); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Consider adding error validation in the _AdminService_DeleteExecutionPhase_Handler function to ensure the input request is not nil before proceeding with the decoding operation.
Code suggestion
Check the AI-generated fix before applying
| in := new(admin.ExecutionPhaseDeleteRequest) | |
| if err := dec(in); err != nil { | |
| return nil, err | |
| if srv == nil { | |
| return nil, status.Error(codes.InvalidArgument, "server interface is nil") | |
| } | |
| in := new(admin.ExecutionPhaseDeleteRequest) | |
| if err := dec(in); err != nil { | |
| return nil, err |
Code Review Run #50b8fe
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #26e38fActionable Suggestions - 6
Additional Suggestions - 1
Review Details
AI Code Review powered by |
| { no: 1, name: "id", kind: "message", T: WorkflowExecutionIdentifier }, | ||
| { no: 2, name: "phase", kind: "enum", T: proto3.getEnumType(WorkflowExecution_Phase) }, |
There was a problem hiding this comment.
Consider updating the field names to be more descriptive. The change from execution_phase to phase may reduce clarity. Perhaps consider keeping the prefix to maintain context and avoid potential naming conflicts.
Code suggestion
Check the AI-generated fix before applying
| { no: 1, name: "id", kind: "message", T: WorkflowExecutionIdentifier }, | |
| { no: 2, name: "phase", kind: "enum", T: proto3.getEnumType(WorkflowExecution_Phase) }, | |
| { no: 1, name: "id", kind: "message", T: WorkflowExecutionIdentifier }, | |
| { no: 2, name: "execution_phase", kind: "enum", T: proto3.getEnumType(WorkflowExecution_Phase) }, |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if executionPhase == core.WorkflowExecution_UNDEFINED { | ||
| return nil, fmt.Errorf("execution phase cannot be undefined") | ||
| } |
There was a problem hiding this comment.
Consider adding more specific validation for the execution phase. The current check only validates against UNDEFINED but there may be other invalid phases. Consider validating against a list of allowed phases.
Code suggestion
Check the AI-generated fix before applying
| if executionPhase == core.WorkflowExecution_UNDEFINED { | |
| return nil, fmt.Errorf("execution phase cannot be undefined") | |
| } | |
| validPhases := []core.WorkflowExecution_Phase{ | |
| core.WorkflowExecution_QUEUED, | |
| core.WorkflowExecution_RUNNING, | |
| core.WorkflowExecution_SUCCEEDING, | |
| core.WorkflowExecution_SUCCEEDED, | |
| core.WorkflowExecution_FAILING, | |
| core.WorkflowExecution_FAILED, | |
| core.WorkflowExecution_ABORTED, | |
| core.WorkflowExecution_TIMED_OUT, | |
| core.WorkflowExecution_ABORTING, | |
| } | |
| isValid := false | |
| for _, phase := range validPhases { | |
| if executionPhase == phase { | |
| isValid = true | |
| break | |
| } | |
| } | |
| if !isValid { | |
| return nil, fmt.Errorf("invalid execution phase: %s", executionPhase) | |
| } |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if workflowExecutionID.Project == "" || workflowExecutionID.Domain == "" { | ||
| return nil, fmt.Errorf("workflow execution identifier must have project, domain") |
There was a problem hiding this comment.
Consider validating the Name field of workflowExecutionID as well. The WorkflowExecutionIdentifier typically requires all fields (Project, Domain, and Name) to be non-empty for a valid identifier.
Code suggestion
Check the AI-generated fix before applying
| if workflowExecutionID.Project == "" || workflowExecutionID.Domain == "" { | |
| return nil, fmt.Errorf("workflow execution identifier must have project, domain") | |
| if workflowExecutionID.Project == "" || workflowExecutionID.Domain == "" || workflowExecutionID.Name == "" { | |
| return nil, fmt.Errorf("workflow execution identifier must have project, domain, and name") |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "phase") | ||
| } | ||
|
|
||
| e, err = runtime.Enum(val, extCore.WorkflowExecution_Phase_value) |
There was a problem hiding this comment.
Consider adding validation for the phase parameter against valid workflow execution phases before attempting enum conversion. The current implementation may allow invalid phase values to be processed.
Code suggestion
Check the AI-generated fix before applying
| e, err = runtime.Enum(val, extCore.WorkflowExecution_Phase_value) | |
| _, exists := extCore.WorkflowExecution_Phase_value[val] | |
| if !exists { | |
| return nil, metadata, status.Errorf(codes.InvalidArgument, "invalid phase value: %s", val) | |
| } | |
| e, err = runtime.Enum(val, extCore.WorkflowExecution_Phase_value) |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| #[prost(message, optional, tag="1")] | ||
| pub id: ::core::option::Option<super::core::WorkflowExecutionIdentifier>, | ||
| #[prost(enumeration="super::core::workflow_execution::Phase", tag="2")] | ||
| pub phase: i32, |
There was a problem hiding this comment.
Consider updating the struct field names to be more descriptive. The field phase could be renamed to execution_phase to better indicate its purpose and maintain consistency with the previous implementation.
Code suggestion
Check the AI-generated fix before applying
| #[prost(message, optional, tag="1")] | |
| pub id: ::core::option::Option<super::core::WorkflowExecutionIdentifier>, | |
| #[prost(enumeration="super::core::workflow_execution::Phase", tag="2")] | |
| pub phase: i32, | |
| #[prost(message, optional, tag="1")] | |
| pub execution_id: ::core::option::Option<super::core::WorkflowExecutionIdentifier>, | |
| #[prost(enumeration="super::core::workflow_execution::Phase", tag="2")] | |
| pub execution_phase: i32, |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| type ExecutionPhaseDeleteInput struct { | ||
| WorkflowExecutionID core.WorkflowExecutionIdentifier | ||
| ExecutionPhase core.WorkflowExecution_Phase | ||
| } |
There was a problem hiding this comment.
Consider adding validation for WorkflowExecutionID and ExecutionPhase fields in ExecutionPhaseDeleteInput struct. These fields appear to be critical for deletion operations and should be validated before use.
Code suggestion
Check the AI-generated fix before applying
| type ExecutionPhaseDeleteInput struct { | |
| WorkflowExecutionID core.WorkflowExecutionIdentifier | |
| ExecutionPhase core.WorkflowExecution_Phase | |
| } | |
| type ExecutionPhaseDeleteInput struct { | |
| WorkflowExecutionID core.WorkflowExecutionIdentifier | |
| ExecutionPhase core.WorkflowExecution_Phase | |
| } | |
| func (i *ExecutionPhaseDeleteInput) Validate() error { | |
| if i.WorkflowExecutionID.Project == "" || i.WorkflowExecutionID.Domain == "" || i.WorkflowExecutionID.Name == "" { | |
| return fmt.Errorf("invalid workflow execution identifier") | |
| } | |
| if i.ExecutionPhase == core.WorkflowExecution_UNDEFINED { | |
| return fmt.Errorf("execution phase cannot be undefined") | |
| } | |
| return nil | |
| } |
Code Review Run #26e38f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
taieeuu
changed the title
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #66451dActionable Suggestions - 0Review Details
AI Code Review powered by |
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run #de2c1dActionable Suggestions - 0Review Details
AI Code Review powered by |
taieeuu
changed the title
Signed-off-by: taieeuu <[email protected]>
Signed-off-by: taieeuu <[email protected]>
Code Review Agent Run Status
|
Signed-off-by: taieeuu <[email protected]>
Future-Outlier
changed the title
Code Review Agent Run Status
|
There was a problem hiding this comment.
- do we want to return how many executions we deleted?
There's a field calledresult.RowsAffected? in result
// DB GORM DB definition
type DB struct {
*Config
Error error
RowsAffected int64
Statement *Statement
clone int
}- should we use soft delete? or hard delete is what we want?
|
|
||
| func (m *AdminService) DeleteExecutionPhase( | ||
| ctx context.Context, request *admin.ExecutionPhaseDeleteRequest) (*admin.ExecutionPhaseDeleteResponse, error) { | ||
| var response *admin.ExecutionPhaseDeleteResponse | ||
| var err error | ||
| m.Metrics.executionEndpointMetrics.terminate.Time(func() { | ||
| response, err = m.ExecutionManager.DeleteExecutionPhase(ctx, request) |
There was a problem hiding this comment.
do you know where to see the metrics?
is it stored in database or a metric server something like that?
There was a problem hiding this comment.
The data is stored in Prometheus, but 'flytectl demo start' does not launch Prometheus.
| message ExecutionPhaseDeleteResponse { | ||
| string message = 1; | ||
| } | ||
|
|
| @@ -430,3 +430,13 @@ message WorkflowExecutionGetMetricsResponse { | |||
| // hierarchical structure using Flyte entity references. | |||
| core.Span span = 1; | |||
| } | |||
|
|
|||
| message ExecutionPhaseDeleteRequest { | |||
There was a problem hiding this comment.
rather than requiring this deletion to be based on phase attributes only (what if one day we want to support timestamp filters instead?) can we make this more generic and re-use the ResourceListRequest (
flyte/flyteidl/protos/flyteidl/admin/common.proto
Lines 203 to 225 in a2331bd
There was a problem hiding this comment.
something like
rpc DeleteExecutions (flyteidl.admin. ResourceListRequest) returns (flyteidl.admin.ExecutionDeleteResponse) {
option (google.api.http) = {
delete: "/api/v1/delete_execution/{id.project}/{id.domain}"
};
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
description: "Delete all executions matching request filters"
};
};
}
|
before we proceed with deleting items from the db can we maybe write up an issue to address
I think as it stands now this PR could orphan executions unintentionally if we're not careful or make it really easy to delete from the DB which is a departure from how flyteadmin currently records entities. Flyteadmin allows us to archive or tombstone various entities but delete is a scarier tool |
Tracking issue
Why are the changes needed?
This API provides users with a method to batch delete unnecessary execution records. It accepts the parameters project, domain, and phase, enabling the deletion of execution records that are no longer needed.
What changes were proposed in this pull request?
In the protobuf definition, define an RPC method called DeleteExecutionPhase that accepts project, domain, and phase parameters and uses the DELETE method for the deletion request. In both execution_manager and execution_repo, implement deletion logic that includes input validation and error handling. Unit tests are provided to ensure correct functionality under both successful and failure scenarios.
How was this patch tested?
"All tests passed successfully, and I have also added unit tests."
And you can also test with postman ... tools .
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR implements a new API endpoint for batch deleting execution phases in Flyte, introducing protobuf message types with implementations in Go, TypeScript, and Rust. It updates execution manager and repository layers, improves encapsulation by replacing direct field access with getter methods, and switches from GET to DELETE methods with proper validation and error handling.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5