Skip to content
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

Get reverse history and set scheduled event task ID properly #246

Conversation

samanbarghi
Copy link
Contributor

What was changed

Get history events in reverse and fix the logic to set to first task scheduled
event instead of the last one.

Why?

Currently, we iterate through the events starting from the beginning and try to
find a workflowTaskCompleted event and keep going until we reach the end of the
list. If we cannot find a completed event, we set the event ID to the id of the
workflowScheduledTask event. However, this means if we have
'completed->completed->failed' or 'failed->retry(failed)->retry(failed)` events,
we always set the event ID to the
scheduled event corresponding to the last failed workflow event. Instead, by
iterating through the events in reverse, we can fix this behavior and set the
last workflowCompletedEvent or the first workflowScheduled event (if all
attempts failed).

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@feedmeapples
Copy link
Contributor

feedmeapples commented May 24, 2023

would like to have an e2e for this if you have capacity

@bergundy
Copy link
Member

Just verifying that this doesn’t change anything in the ux

@samanbarghi
Copy link
Contributor Author

would like to have an e2e for this if you have capacity

adding e2e tests will be a bit involved as we will need to generate many scenarios, the same logic exists in the server and there are unit-tests for it, see this PR.

I am not sure if e2e testing this specific function have any benefits now, will leave this to be included in e2e tests for reset subcommand.

@samanbarghi
Copy link
Contributor Author

Just verifying that this doesn’t change anything in the ux

It should not, there is an enum used in ux to select among different methods.

Btw, we might want to consider using BatchReset API added to server recently in this PR.

@feedmeapples
Copy link
Contributor

Btw, we might want to consider using BatchReset API added to server recently in this PR.

That's great to hear!

@feedmeapples feedmeapples merged commit 30704e3 into temporalio:main Jun 21, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants