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

fix(task): inactive task runs when updated #22211

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

raffs
Copy link
Contributor

@raffs raffs commented Aug 15, 2021

Closes #21874

When a tasks is being updated, ensure is scheduled only when the tasks status is active. Otherwise, an already-inactivated tasks might be scheduled if the task status is not being changed.

@raffs raffs force-pushed the fix/inative-task-runs-update branch from b1d33ca to 96f8ff8 Compare August 16, 2021 04:27
@williamhbaker williamhbaker self-requested a review August 16, 2021 20:43
@williamhbaker
Copy link
Contributor

Great job tracking this down @raffs!

One other thing you'll need to do is add a test case to show that the bug is fixed. I think you could create a new case in the existing Test_Coordinator_Scheduler_Methods test that shows the expected scheduler calls for updating an inactive task without changing it's status.

@raffs raffs force-pushed the fix/inative-task-runs-update branch from 96f8ff8 to 12bf341 Compare August 17, 2021 07:41
@raffs
Copy link
Contributor Author

raffs commented Aug 17, 2021

Great job tracking this down @raffs!

One other thing you'll need to do is add a test case to show that the bug is fixed. I think you could create a new case in the existing Test_Coordinator_Scheduler_Methods test that shows the expected scheduler calls for updating an inactive task without changing it's status.

Thanks @wbaker85.

I have added the test case as requested. Previously, I wasn't entire sure how to create the test case for a tasks that shouldn't be scheduled. But now, I can see that we diff the config on every test and having a empty schedule for updating an already inactive tasks seems to do trick for testing this case.

@danxmoran danxmoran self-requested a review August 17, 2021 17:44
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

LGTM! The CI failure is unrelated to this PR, we added a new job that apparently doesn't work on PRs from forked repos. We'll patch it up and then a rebase/merge from master should get this passing.

@williamhbaker
Copy link
Contributor

williamhbaker commented Aug 17, 2021

@raffs - I merged #22232 just now, so if you can rebase onto the latest master & resolve the conflict in the changelog we should be able to get this merged!

@raffs raffs force-pushed the fix/inative-task-runs-update branch from 12bf341 to cd39740 Compare August 17, 2021 18:58
@williamhbaker williamhbaker merged commit 991ff02 into influxdata:master Aug 17, 2021
@raffs
Copy link
Contributor Author

raffs commented Aug 17, 2021

LGTM! The CI failure is unrelated to this PR, we added a new job that apparently doesn't work on PRs from forked repos. We'll patch it up and then a rebase/merge from master should get this passing.

Thank you for the info @danxmoran

@raffs - I merged #22232 just now, so if you can rebase onto the latest master & resolve the conflict in the changelog we should be able to get this merged!

Great! thanks for the info.

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.

[bug]: Saving an inactive task activates it without toggling it to active state
3 participants