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

Add "cancel_timeout" parameter #21

Merged
merged 7 commits into from
Sep 9, 2024

Conversation

lowellausen
Copy link
Member

We have faced the need for variable timeout values when waiting for a task to cancel.

For example: if you have a record video task that compresses a video and saves when you press cancel. It can take several seconds to do that, and, with the current implementation, the wait for cancel will timeout and the task will succeed; although the cancelling keeps going on and the task result will eventually be sent.

Changes:

  • Removed the currently hard-coded 5 seconds timeout when waiting for a task to be cancelled
  • Added a new parameter to make that timeout variable
    • The default value is still the same 5 seconds as before
  • Removed another 5 seconds timeout that was hard-coded on the task registrator, that was simply overwriting the previously mentioned hard-coded timeout
  • Had to change a test case:
    • Added the new parameter to one of the tasks being created on the integration tests
    • Added cancel_timeout of 1.5 seconds to it
    • Changed the test that creates a task that should take longer than the allowed timeout. Has to be 2 seconds now (the run_time_secs has to be an int due to its hacky nature)

Leonardo Oliveira Wellausen added 2 commits August 1, 2024 12:34
@lowellausen lowellausen requested review from Jannkar and jonipol August 8, 2024 06:38
@lowellausen lowellausen force-pushed the ros-3295-cancel-timeout branch from af4c337 to ff4d481 Compare August 8, 2024 06:57
Copy link
Contributor

@jonipol jonipol left a comment

Choose a reason for hiding this comment

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

A small cleanup suggestions. Otherwise pretty straight forward change. LGTM!

lowellausen and others added 3 commits August 9, 2024 13:53
…ros-3295-cancel-timeout

# Conflicts:
#	task_manager/task_manager/task_client.py
Copy link
Collaborator

@Jannkar Jannkar left a comment

Choose a reason for hiding this comment

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

Looks good! Nice to have this value parameterized. Couple of the tests seem to be failing after the new changes.

@lowellausen
Copy link
Member Author

Looks good! Nice to have this value parameterized. Couple of the tests seem to be failing after the new changes.

Took me a while to come back to this, but fixed the tests now!

@lowellausen lowellausen requested a review from Jannkar September 4, 2024 09:22
@lowellausen lowellausen merged commit 73ca98b into Karelics:main Sep 9, 2024
1 check passed
@Jannkar
Copy link
Collaborator

Jannkar commented Sep 10, 2024

Sorry, didn't have time to recheck the changes until now. Looks good!

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.

3 participants