-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(workflow): enhance request status handling and workflow cleanup #898
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
|
Caution Review failedThe pull request is closed. WalkthroughNew mechanisms for workflow cleanup and status management were introduced. A centralized function for default status messages was added. Workflow completion now triggers cleanup of the workflow chain. Status computation and updates were refactored for clarity, and progress reporting was enhanced with weighted calculations. Several method signatures were updated or added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkflowCoordinator
participant RequestService
participant OperationHandler
Client->>WorkflowCoordinator: CompleteWorkflowStep(requestID)
WorkflowCoordinator->>RequestService: GetRequest(requestID)
WorkflowCoordinator->>RequestService: GetRequestMetadata(requestID)
alt Workflow not disabled
loop For each request in workflow chain
WorkflowCoordinator->>RequestService: GetRequest(currentRequestID)
WorkflowCoordinator->>RequestService: DeleteRequest(currentRequestID)
end
end
WorkflowCoordinator->>Client: Return completion or error
sequenceDiagram
participant Client
participant RequestService
participant OperationHandler
Client->>RequestService: ExecuteRequest(requestID)
RequestService->>RequestService: ComputeRequestStatus(requestID)
RequestService->>RequestService: UpdateRequestStatus(requestID, status)
RequestService->>OperationHandler: Execute(request)
RequestService->>RequestService: ComputeRequestStatus(requestID)
RequestService->>RequestService: UpdateRequestStatus(requestID, status)
RequestService->>Client: Return result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/request.go
(1 hunks)core/workflow.go
(1 hunks)service/request.go
(9 hunks)service/workflow.go
(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
core/request.go (1)
db/models/request.go (5)
RequestStatusType
(10-10)RequestStatusPending
(14-14)RequestStatusProcessing
(15-15)RequestStatusCompleted
(16-16)RequestStatusFailed
(17-17)
service/request.go (3)
db/models/request.go (2)
RequestStatusType
(10-10)RequestStatusFailed
(17-17)core/context.go (1)
Context
(18-46)core/request.go (2)
RequestStatus
(61-67)GetDefaultStatusMessage
(70-83)
service/workflow.go (3)
core/workflow.go (1)
WorkflowStatus
(328-338)db/models/request.go (1)
RequestStatusCompleted
(16-16)core/context.go (1)
Context
(18-46)
🔇 Additional comments (9)
core/workflow.go (1)
48-49
: Interface extension looks good!The new
CleanupWorkflow
method is a well-designed addition to the interface, providing a clear contract for cleaning up workflow resources.core/request.go (1)
69-83
: Clean implementation of centralized status messages!The
GetDefaultStatusMessage
function provides a single source of truth for default status messages, improving consistency across the codebase.service/request.go (5)
145-157
: Improved status tracking during execution!The enhanced execution flow now properly computes and updates the request status both before and after handler execution, providing more accurate status tracking throughout the operation lifecycle.
Also applies to: 181-188
324-324
: Good addition of timestamp tracking!Updating the
updated_at
field ensures accurate tracking of when status changes occur.
341-341
: Correct method usage after renaming.The change to use
ComputeRequestStatus
aligns with the refactored status computation logic.
365-406
: Well-structured status computation logic!The refactored
ComputeRequestStatus
method provides a clear hierarchy for status determination:
- Detailed status from operation handler (if available)
- Basic status from request model (fallback)
- Centralized default messages with special handling for failure reasons
This improves status reporting consistency and accuracy.
423-442
: Clear separation of status retrieval methods!The new
GetRequestStatus
method provides a lightweight way to retrieve stored status without querying handlers, complementing the more comprehensiveComputeRequestStatus
method.service/workflow.go (2)
391-400
: Automatic cleanup on workflow completion!Good addition of automatic cleanup when workflows complete. The error handling appropriately logs failures while still propagating the error.
547-580
: Enhanced progress tracking with weighted calculation!The new weighted progress calculation provides more accurate progress reporting by combining:
- 50% weight from step progression through the workflow
- 50% weight from the current step's internal progress
This gives users better visibility into both macro (workflow) and micro (step) progress.
- Added GetDefaultStatusMessage helper function to core.RequestStatus - Implemented ComputeRequestStatus to consolidate status calculation logic - Improved workflow status tracking with weighted progress calculation - Added CleanupWorkflow method to WorkflowCoordinator interface - Enhanced workflow completion handling with automatic cleanup - Updated request status updates to include timestamp refresh - Separated GetRequestStatus from ComputeRequestStatus for clarity - Improved error handling throughout workflow operations - Added better logging for workflow transitions and cleanup Key changes: - Status calculation now combines step progress (50%) and internal operation progress (50%) - Workflows now automatically trigger cleanup upon completion - More robust status message handling using default messages - Better separation between status computation and status retrieval
77a2914
to
89db14a
Compare
Key changes:
Summary by CodeRabbit
New Features
Improvements