Skip to content
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.

Sign up for GitHub

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

[WIP] [Feature] add batch delete execution_phase api #6267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1,338 −361
Open

[WIP] [Feature] add batch delete execution_phase api #6267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions flyteadmin/pkg/manager/impl/execution_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1956,3 +1956,16 @@ func addStateFilter(filters []common.InlineFilter) ([]common.InlineFilter, error
}
return filters, nil
}

func (m *ExecutionManager) DeleteExecutionPhase(ctx context.Context, req *admin.ExecutionPhaseDeleteRequest) (*admin.ExecutionPhaseDeleteResponse, error) {
executionPhase := req.GetExecutionPhase()

err := m.db.ExecutionRepo().Delete(ctx, executionPhase)
if err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding context to error return

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
Suggested change
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

}

return &admin.ExecutionPhaseDeleteResponse{
Message: fmt.Sprintf("Execution %s deleted successfully", executionPhase),
}, nil
}
1 change: 1 addition & 0 deletions flyteadmin/pkg/manager/interfaces/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,5 @@ type ExecutionInterface interface {
ListExecutions(ctx context.Context, request *admin.ResourceListRequest) (*admin.ExecutionList, error)
TerminateExecution(
ctx context.Context, request *admin.ExecutionTerminateRequest) (*admin.ExecutionTerminateResponse, error)
DeleteExecutionPhase(ctx context.Context, request *admin.ExecutionPhaseDeleteRequest) (*admin.ExecutionPhaseDeleteResponse, error)
}
8 changes: 8 additions & 0 deletions flyteadmin/pkg/repositories/gormimpl/execution_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,11 @@ func NewExecutionRepo(
metrics: metrics,
}
}

func (r *ExecutionRepo) Delete(ctx context.Context, executionPhase string) error {
result := r.db.Delete(&models.Execution{}, "phase = ?", executionPhase)
if result.Error != nil {
return result.Error
}
return nil
Comment on lines +175 to +179
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding validation for execution phase

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
Suggested change
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

}
1 change: 1 addition & 0 deletions flyteadmin/pkg/repositories/interfaces/execution_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type ExecutionRepoInterface interface {
List(ctx context.Context, input ListResourceInput) (ExecutionCollectionOutput, error)
// Returns count of executions matching query parameters.
Count(ctx context.Context, input CountResourceInput) (int64, error)
Delete(ctx context.Context, executionPhase string) error
}

// Response format for a query on workflows.
Expand Down
14 changes: 14 additions & 0 deletions flyteadmin/pkg/rpc/adminservice/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,17 @@ func (m *AdminService) TerminateExecution(
m.Metrics.executionEndpointMetrics.terminate.Success()
return response, nil
}

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)
Comment on lines +160 to +161
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using specific metric for DeleteExecutionPhase

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

})
if err != nil {
return nil, util.TransformAndRecordError(err, &m.Metrics.executionEndpointMetrics.terminate)
}
m.Metrics.executionEndpointMetrics.terminate.Success()
return response, nil
}
48 changes: 48 additions & 0 deletions flyteidl/clients/go/admin/mocks/AdminServiceClient.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions flyteidl/clients/go/admin/mocks/AdminServiceServer.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions flyteidl/clients/go/assets/admin.swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 74 additions & 0 deletions flyteidl/gen/pb-es/flyteidl/admin/execution_pb.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion flyteidl/gen/pb-es/flyteidl/service/admin_connect.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading