-
Notifications
You must be signed in to change notification settings - Fork 2
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
.cs files #42
base: main
Are you sure you want to change the base?
.cs files #42
Conversation
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.
Just did high-level review of Temporal-specific things and ignored the other files
|
||
public static class TemporalClientExtensions | ||
{ | ||
public static async Task<bool> IsWorkflowRunning<T>(this ITemporalClient client, string workflowId) |
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.
No value in the type parameter here or in the handle extensions
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.
Added for ease of use and to reduce duplication. It calls another extension with a handle statuses check
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.
public static async Task<bool> IsWorkflowRunning<T>(this ITemporalClient client, string workflowId) | |
public static async Task<bool> IsWorkflowRunning(this ITemporalClient client, string workflowId) |
I am saying everywhere in these extensions you are accepting a WorkflowHandle
with a type parameter unnecessarily, you can just accept WorkflowHandle
.
catch (RpcException ex) | ||
{ | ||
if (ex.Code is RpcException.StatusCode.NotFound) | ||
return false; | ||
|
||
throw; | ||
} |
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.
catch (RpcException ex) | |
{ | |
if (ex.Code is RpcException.StatusCode.NotFound) | |
return false; | |
throw; | |
} | |
catch (RpcException ex) when (ex.Code == RpcException.StatusCode.NotFound) | |
{ | |
return false; | |
} |
Easier to read
|
||
_unitOfWork.Repository<DAL.Entities.Flight>().Update(flight); | ||
|
||
await _unitOfWork.SaveChangesAsync(); |
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.
Should pass activity execution context's cancellation token to downstream calls
} | ||
|
||
[Activity] | ||
public Task<FlightDetailsModel> AssignSeats(FlightDetailsModel flight) |
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.
You don't have to make these return Task
if they don't do anything async
[Workflow] | ||
public class FlightWorkflow | ||
{ | ||
private readonly ActivityOptions _activityOptions = new() |
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.
Can make static
_saga.OnCompensationError(log => | ||
{ | ||
log.Add("Compensation error. Manual intervention required!"); | ||
|
||
return Task.CompletedTask; | ||
}); | ||
|
||
_saga.OnCompensationComplete(log => | ||
{ | ||
log.Add("Compensation completed successfully"); | ||
|
||
return Task.CompletedTask; | ||
}); |
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.
These seem like things that should be part of the Saga construction, not done in the exception handler
{ | ||
_saga.OnCompensationError(log => | ||
{ | ||
log.Add("Compensation error. Manual intervention required!"); |
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.
Usually if manual intervention is required, the workflow is put in a state where it can be done. If the workflow completes (even a failure), people are not likely to see that they need to do something
RetryPolicy = new Temporalio.Common.RetryPolicy | ||
{ | ||
} |
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.
RetryPolicy = new Temporalio.Common.RetryPolicy | |
{ | |
} |
This is the same as leaving it unset from a behavior POV
StartToCloseTimeout = TimeSpan.FromSeconds(60), | ||
RetryPolicy = new RetryPolicy | ||
{ | ||
InitialInterval = TimeSpan.FromSeconds(15), |
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.
Usually when altering activities to change from the default, it's not a global change but specific to an activity
|
||
try | ||
{ | ||
_log.Add($"Attempting compensation {i}..."); |
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.
Might as well use a real logger instead of a string list
No description provided.