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

Use status for queue fix and optimization #721

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Conversation

chuck-dbos
Copy link
Collaborator

@chuck-dbos chuck-dbos commented Jan 16, 2025

When starting a workflow, look at any existing record in the workflow status table.
If the workflow is already complete (SUCCESS or ERROR) do nothing (let the caller retrieve the result from the handle)
If the workflow is now ENQUEUED, ensure there is a workflow_queue record for it, but do not add such records if the workflow has moved past that state.

@chuck-dbos chuck-dbos self-assigned this Jan 16, 2025
Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Great catch and fix, this looks good! (and makes a ton of sense)

Could we also add a new test for a scenario like that in #706 both here and in Python?

@chuck-dbos chuck-dbos marked this pull request as ready for review January 17, 2025 21:40
@chuck-dbos chuck-dbos changed the title Use status for queue fix, and long-overdue optimization Use status for queue fix and optimization Jan 17, 2025
@chuck-dbos
Copy link
Collaborator Author

Could we also add a new test for a scenario like that in #706 both here and in Python?

I added a test now that failed on old and passes on new. A bit of unnatural act there, let me know if you have better ideas on how to do this. Sorry for the clutter too, I updated the test to use v2 API because it is just much easier to launch() and shutdown().

Python is ongoing, the branch exists, I've tested the same thing manually, and need to automate.

Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Great fix!

@chuck-dbos chuck-dbos merged commit 626cee1 into main Jan 17, 2025
9 of 10 checks passed
@chuck-dbos chuck-dbos deleted the chuck/wfinitstat branch January 17, 2025 22:36
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.

2 participants