Fail QueryWorkflow if last workflow task failed #4099
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
samanbarghi
force-pushed
the
bugfix/fail-query-fast-if-workflow-task-has-failed
branch
from
4e9acc3 to
61c0227
Compare
alexshtin
changed the title
yiminc
reviewed
Mar 24, 2023
| }) | ||
|
|
||
| if err != nil { | ||
| s.Logger.Fatal("SetQueryHandler failed: " + err.Error()) |
Member
There was a problem hiding this comment.
I think Fatel may exit the process or fail the test?
| if mutableState.GetExecutionInfo().WorkflowTaskAttempt > 1 { | ||
| // while workflow task is failing, the query to that workflow will also fail. Failing fast here to prevent wasting | ||
| // resources to load history for a query that will fail. | ||
| return nil, serviceerror.NewFailedPrecondition("Query has failed due to a failing workflow task") |
Member
There was a problem hiding this comment.
Cannot query workflow due to Workflow Task in failed state.
yycptt
reviewed
Mar 24, 2023
| s.True(workflowRun.GetRunID() != "") | ||
|
|
||
| // wait for workflow to fail | ||
| time.Sleep(time.Second * 5) |
Member
There was a problem hiding this comment.
nit: do we have to wait for 5 sec? seems a bit long. I think workflow task attempt will be larger than 1 as soon as the first workflow task is failed.
yycptt
approved these changes
Mar 24, 2023
Member
yycptt
left a comment
yycptt
left a comment
There was a problem hiding this comment.
Need this fix for 1.20.1 patch release.
yycptt
added a commit
that referenced
this pull request
Mar 25, 2023
* Fail QueryWorkflow if last WorkflowTask failed * fix linter issues * pr comments --------- Co-authored-by: Saman Barghi <[email protected]> Co-authored-by: Yichao Yang <[email protected]>
samanbarghi
deleted the
bugfix/fail-query-fast-if-workflow-task-has-failed
branch
wxing1292
pushed a commit
that referenced
this pull request
Apr 14, 2023
* Fail QueryWorkflow if last WorkflowTask failed * fix linter issues * pr comments --------- Co-authored-by: Saman Barghi <[email protected]> Co-authored-by: Yichao Yang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Fail fast for query if the last workflow task has failed.
Why?
While workflow task is in failing state (due to non-deterministic error etc), the query to those workflow will fail anyway, and deliver query to the workflow will only waste resources to load history.
We should fail fast for query if we know the workflow task would fail.
How did you test it?
New functional test.
Potential risks
No risks.
Is hotfix candidate?
No.