Removed EffectType #115
Removed EffectType #115
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the EffectType enum from the codebase, simplifying effect identification by eliminating type distinctions (Effect, State, System, Timeout, Retry). All effects are now treated uniformly with a single 'E' prefix in their serialized form.
Key Changes:
- Removed the
EffectTypeenum and its extension methods - Updated
EffectIdto remove type parameter, always using 'E' prefix during serialization - Modified timeout and effect detection logic to use value-based parsing instead of type checking
- Added
SerializedEffectIdfield toStoredEffectfor serialization optimization
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Domain/EffectType.cs | Deleted entire file containing EffectType enum and extensions |
| Domain/EffectId.cs | Removed EffectType parameter from record and methods, hardcoded 'E' prefix |
| Domain/Effect.cs | Removed EffectType arguments from all method calls |
| Domain/EffectResults.cs | Removed EffectType parameters from InnerCapture methods, added missing braces |
| Domain/ExistingEffects.cs | Changed GetStoredEffects to internal, updated RemoveFailed logic, used factory methods |
| Domain/ExistingRegisteredTimeouts.cs | Changed from type-based to value-based timeout detection with validation |
| Domain/RetryPolicy.cs | Removed EffectType arguments, replaced Upserts batch call with individual Upsert calls |
| Storage/Types.cs | Added SerializedEffectId field to StoredEffect, updated factory methods and serialization |
| CoreRuntime/FlowRegisteredTimeouts.cs | Removed EffectType arguments, added value-based timeout detection logic |
| CoreRuntime/Invocation/Workflow.cs | Removed EffectType argument from timeout creation |
| CoreRuntime/Invocation/InvocationHelper.cs | Updated MapInitialEffects to include SerializedEffectId field |
| Reactive/Extensions/InnerOperators.cs | Removed EffectType arguments from timeout operators |
| Tests/**.cs | Updated all test cases to remove EffectType parameters, removed type-specific tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Remove all effects - this clears failed effects and any retry-tracking state | ||
| // Since we no longer distinguish effect types, we remove everything to ensure | ||
| // a clean slate for restart |
There was a problem hiding this comment.
The comment suggests all effects are removed to ensure 'a clean slate for restart', but the code only removes effects whose status is Failed or whose type was Retry (which no longer exists). This comment is misleading as it implies all effects are removed unconditionally. The comment should accurately describe that only specific effects are removed based on their status or previous classification.
| // Remove all effects - this clears failed effects and any retry-tracking state | |
| // Since we no longer distinguish effect types, we remove everything to ensure | |
| // a clean slate for restart | |
| // Remove all effects, regardless of their status or previous classification, | |
| // to ensure a clean slate for restart. |
|
|
||
| var effectIds = (await effects.AllIds).ToList(); | ||
| var timeoutIds = effectIds.Where(id => id.Type == EffectType.Timeout); | ||
| var allEffects = await effects.GetStoredEffects(); |
There was a problem hiding this comment.
Fetching all effects to find timeouts is inefficient. Previously, the code filtered by EffectType.Timeout using AllIds. Now it retrieves all stored effects (which may include full result payloads) and then filters by parsing values. Consider retrieving only effect IDs first, then fetching values for potential timeout effects to avoid loading unnecessary data.
| public static async Task<IReadOnlyList<RegisteredTimeout>> PendingTimeouts(Effect effect) | ||
| { | ||
| var timeoutIds = effect.EffectIds.Where(id => id.Type == EffectType.Timeout); | ||
| var timeoutIds = effect.EffectIds; |
There was a problem hiding this comment.
This change retrieves all effect IDs instead of filtering for timeout effects first. Combined with subsequent value parsing for each effect, this creates unnecessary overhead when many non-timeout effects exist. Consider a more efficient approach to identify timeout effects without checking every single effect ID.
| var timeoutIds = effect.EffectIds; | |
| var timeoutIds = effect.EffectIds.Where(id => id.Value.StartsWith("timeout_")); |
| await effect.Upsert(delayUntilId, delayUntil.Ticks, flush: false); | ||
| await effect.Upsert(iterationId, iteration, flush: false); |
There was a problem hiding this comment.
Replacing a single batch Upserts call with two sequential Upsert calls may impact performance. If both operations can be batched together, the original approach of using Upserts with a collection would be more efficient for reducing database round-trips or storage operations.
| await effect.Upsert(delayUntilId, delayUntil.Ticks, flush: false); | |
| await effect.Upsert(iterationId, iteration, flush: false); | |
| await effect.Upserts(new[] { | |
| (delayUntilId, (object)delayUntil.Ticks), | |
| (iterationId, (object)iteration) | |
| }, flush: false); |
No description provided.