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

Remove retrying and submit-retrying states #526

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Nov 3, 2020

These changes close #514

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Already covered by existing tests.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow
Copy link
Member Author

kinow commented Nov 3, 2020

The checkpoint workflow, used to generate the data for the offline mode uses the retrying state too. So I will need a bit more of time understanding how to rewrite it with an xtrigger (I think that's the replacement).

@kinow kinow self-assigned this Nov 3, 2020
@kinow kinow added this to the 0.3 milestone Nov 3, 2020
@codecov-io
Copy link

Codecov Report

Merging #526 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
- Coverage   68.02%   67.96%   -0.07%     
==========================================
  Files          44       44              
  Lines         954      952       -2     
  Branches       59       59              
==========================================
- Hits          649      647       -2     
  Misses        288      288              
  Partials       17       17              
Flag Coverage Δ
unittests 67.96% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/model/TaskState.model.js 100.00% <ø> (ø)
src/utils/tasks.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61c4852...4f2ce48. Read the comment docs.

@hjoliver
Copy link
Member

hjoliver commented Nov 3, 2020

The checkpoint workflow, used to generate the data for the offline mode uses the retrying state too. So I will need a bit more of time understanding how to rewrite it with an xtrigger (I think that's the replacement).

Maybe that's not necessary at this point, until we can represent xtriggers in the UI just use waiting as that is the replacement for retrying in terms of state (it is a waiting task, that is waiting on an xtrigger). So a task that fails and retries several times before succeeding (or failing) will look like this: running -> waiting -> running -> waiting -> succeeded (or failed).

@kinow
Copy link
Member Author

kinow commented Nov 3, 2020

@hjoliver

Maybe that's not necessary at this point, until we can represent xtriggers in the UI just use waiting as that is the replacement for retrying in terms of state (it is a waiting task, that is waiting on an xtrigger). So a task that fails and retries several times before succeeding (or failing) will look like this: running -> waiting -> running -> waiting -> succeeded (or failed).

👍 I replaced retrying and submit-retrying by waiting in our tests already. I thought that the workflow used to generate offline data wouldn't work after the latest changes, and I would have to update it.

But I think you are correct!

But I just tried it, and it executes successfully, producing the JavaScript data for the offline mode 🎉 So nothing else to be done here. Thanks!

@kinow kinow marked this pull request as ready for review November 3, 2020 20:22
@kinow kinow requested a review from hjoliver November 4, 2020 21:41
@kinow
Copy link
Member Author

kinow commented Nov 4, 2020

@hjoliver assigning you just in case you have time for a quick review today 😬 similar to the other one you reviewed yesterday.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hjoliver hjoliver merged commit 0b823e3 into cylc:master Nov 9, 2020
@kinow kinow deleted the remove-retrying-state branch November 9, 2020 03:04
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.

Remove retrying and submit-retrying states
3 participants