Skip to content

Commit

Permalink
Reduce logging on WFTCompletedHandler.Invoke error (#7195)
Browse files Browse the repository at this point in the history
## What changed?
<!-- Describe what has changed in this PR -->
Reduce logging on `WFTCompletedHandler.Invoke` error. Now it is emitted
only when effects were actually cleared.

## Why?
<!-- Tell your future self why have you made these changes -->
Remove noisy logs.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Didn't test.

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
No risks.

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
No.

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
No.

---------

Co-authored-by: Stephan Behnke <[email protected]>
  • Loading branch information
alexshtin and stephanos authored Jan 31, 2025
1 parent 9fe70b4 commit 3dd9794
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 15 deletions.
12 changes: 10 additions & 2 deletions internal/effect/buffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,28 @@ func (b *Buffer) OnAfterRollback(effect func(context.Context)) {

// Apply invokes the buffered effect functions in the order that they were added
// to this Buffer.
func (b *Buffer) Apply(ctx context.Context) {
// It returns true if any effects were applied.
func (b *Buffer) Apply(ctx context.Context) bool {
applied := false
b.cancels = nil
for _, effect := range b.effects {
effect(ctx)
applied = true
}
b.effects = nil
return applied
}

// Cancel invokes the buffered rollback functions in the reverse of the order
// that they were added to this Buffer.
func (b *Buffer) Cancel(ctx context.Context) {
// It returns true if any effects were canceled.
func (b *Buffer) Cancel(ctx context.Context) bool {
canceled := false
b.effects = nil
for i := len(b.cancels) - 1; i >= 0; i-- {
b.cancels[i](ctx)
canceled = true
}
b.cancels = nil
return canceled
}
24 changes: 11 additions & 13 deletions service/history/api/respondworkflowtaskcompleted/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,20 +237,17 @@ func (handler *WorkflowTaskCompletedHandler) Invoke(

var effects effect.Buffer
defer func() {
// code in this file and workflowTaskHandler is inconsistent in the way
// errors are returned - some functions which appear to return error
// actually return nil in all cases and instead set a member variable
// that should be observed by other collaborating code (e.g.
// workflowtaskHandler.workflowTaskFailedCause). That made me paranoid
// about the way this function exits so while we have this defer here
// there is _also_ code to call effects.Cancel at key points.
// `effects` are canceled immediately on WFT failure or persistence errors.
// This `defer` handles rare cases where an error is returned but the cancellation didn't happen.
if retError != nil {
handler.logger.Info("Cancel effects due to error.",
tag.Error(retError),
tag.WorkflowID(token.GetWorkflowId()),
tag.WorkflowRunID(token.GetRunId()),
tag.WorkflowNamespaceID(namespaceEntry.ID().String()))
effects.Cancel(ctx)
cancelled := effects.Cancel(ctx)
if cancelled {
handler.logger.Info("Canceled effects due to error.",
tag.Error(retError),
tag.WorkflowID(token.GetWorkflowId()),
tag.WorkflowRunID(token.GetRunId()),
tag.WorkflowNamespaceID(namespaceEntry.ID().String()))
}
}
}()

Expand Down Expand Up @@ -518,6 +515,7 @@ func (handler *WorkflowTaskCompletedHandler) Invoke(
}

var newWorkflowTask *workflow.WorkflowTaskInfo

// Speculative workflow task will be created after mutable state is persisted.
if newWorkflowTaskType == enumsspb.WORKFLOW_TASK_TYPE_NORMAL {
versioningStamp := request.WorkerVersionStamp
Expand Down

0 comments on commit 3dd9794

Please sign in to comment.