update cancellation sample #276
Conversation
cancellation/activity.go
Outdated
| case <-ctx.Done(): | ||
| logger.Info("context is cancelled") | ||
| return "I am canceled by Done", nil | ||
| return "I am canceled by Done", ctx.Err() |
There was a problem hiding this comment.
No reason to return a value if you're returning cancellation
cancellation/workflow.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| logger.Info(fmt.Sprintf("ActivityToBeCanceled returns %v, %v", result, err)) |
There was a problem hiding this comment.
No reason to log an error you know is nil
cancellation/workflow.go
Outdated
| err = workflow.ExecuteActivity(ctx, a.ActivityToBeSkipped).Get(ctx, nil) | ||
| logger.Error("Error from ActivityToBeSkipped", "Error", err) |
There was a problem hiding this comment.
The original purpose of this sample was to show that this is skipped because the context is already cancelled. Now that we return early unlike before, this is essentially unreachable code. Do you want to remove it?
There was a problem hiding this comment.
done, also added cleanup activity specific activity options (as it can fail heartbeat timeout, and its not heartbeating anyways)
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
tsurdilo
force-pushed
the
upcancellationsample
branch
from
a33890b to
2429adb
Compare
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
May want @Quinn-With-Two-Ns to take a peek also
| return err | ||
| } | ||
|
|
||
| logger.Info("Workflow Execution complete.") |
There was a problem hiding this comment.
Unreachable code, but in this case I think it's fine
|
@tsurdilo did you want to merge this? |
Updates cancellation sample: