-
-
Notifications
You must be signed in to change notification settings - Fork 402
Add plan document for async business rules priority execution #4773
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: rockfordlhotka <[email protected]>
…ocode Co-authored-by: rockfordlhotka <[email protected]>
Co-authored-by: rockfordlhotka <[email protected]>
Co-authored-by: rockfordlhotka <[email protected]>
|
How is the "grouping" inteded to be? Will R1, R2 and R3 run in parallel or serial before proceeding. |
mtavares628
left a comment
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.
I think this solution will work well, and solve a lot of issues that I'm currently having with dependent async rules. How soon could this change be implemented?
| Break (don't process higher priority rules) | ||
| ``` | ||
|
|
||
| ### 5. Alternative Approach: Priority-Group-Based Execution |
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.
I like this approach better, since it allows for sync and async rules at the same priority to run in the intended order instead of processing sync rules first and then async rules.
| 1. **Should serial async rules at the same priority run in sequence or can they run in parallel with each other?** | ||
| - Option A: All serial rules at same priority run one after another | ||
| - Option B: Serial rules at same priority can run in parallel, but all must complete before next priority | ||
| - Recommendation: Option A for maximum control |
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.
I would go with Option A to maintain consistency and give full control.
| 2. **How to handle a mix of sync and async rules at the same priority?** | ||
| - Option A: Run sync first, then async | ||
| - Option B: Maintain original order within priority group | ||
| - Recommendation: Option B to preserve developer intent |
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.
I also vote for option B here.
|
|
||
| 3. **Should there be a timeout for serial async rules?** | ||
| - Could prevent one slow rule from blocking the entire validation | ||
| - Could use existing `CheckRulesAsync` timeout mechanism |
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.
I like the idea of it having a timeout similar to CheclRulesAsync.
| 4. **Should the `ProcessThroughPriority` property affect serial async rules?** | ||
| - Current behavior allows continuing past broken rules up to a certain priority | ||
| - Should serial async rules respect this? | ||
| - Recommendation: Yes, maintain consistent behavior |
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.
I think ProcessThroughPriority should affect serial async rules for consistency.
| - No changes to rule interfaces required for basic usage | ||
|
|
||
| ### Medium Risk | ||
| - The internal `RunRules` method signature may need to become async |
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.
By making this async, it will also have the added benefit of awaiting for RunRules to finish before cycling through and running rules on affected properties. Currently, affected property rules are potentially being executed before the primary async rule has finished executing and as a result the affected property rules are using stale values from other properties that haven't been outputted yet from the original rule.
Reminder: Make sure to review CONTRIBUTING.MD and make sure you've sent in your signed contributor agreement.
Reminder: Add issue number for work item (e.g. #123)
Description
Async rules currently fire in parallel regardless of priority, defeating short-circuiting behavior. A priority-0 async rule failure doesn't prevent priority-1 async rules from starting since they've already been dispatched.
This PR adds a plan document (
docs/dev/Async-Rule-Priority-Execution-Plan.md) proposing:AsyncRuleExecutionModeenum withParallel(default, backward-compatible) andSerialoptionsAsyncExecutionModeproperty onIBusinessRuleBaseinterfaceRunRulesthat:The plan includes:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.