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

TaskBase - Do not sleep after success #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ygalblum
Copy link
Contributor

@ygalblum ygalblum commented Jun 1, 2021

The previous implementation got the status slept and then checked the status
Instead, get the status check it and sleep only if needed

The previous implementation got the status slept and then checked the status
Instead, get the status check it and sleep only if needed
@ygalblum ygalblum requested a review from saimonation June 1, 2021 17:32
@saimonation
Copy link
Collaborator

@ygalblum - is this mandatory? many of the background tasks falsely report "success" until the task actually begins. as a result, the code returns too early. I think all functions that "wait" must be tested to approve this PR

@ygalblum
Copy link
Contributor Author

ygalblum commented Jun 2, 2021

@saimonation
Doron is implementing a script that goes over a long list of paths and runs delete or undelete on them. According to him, the Portal should not have too many tasks running, so he added a wait after each operation. But, then we saw that it always waits for a second.

So, I looked into the implementation and saw that it was waiting needlessly because it first gets the status then waits then checks the status it got. So, it will always return a second after the task is completed. So, even if you are correct in your comment the code doesn't really compensate for it. In order to do that it should have waited before getting the status.

I tested it with the services.connect API and it worked.

If you think this is still an issue, I think we should fix the implementation here and wait before getting the status.
Then I will add an API for waiting for a list of tasks. This will allow Doron to start multiple tasks with one wait (this way the second's wait is not multiplied by the amount of tasks)

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

2 participants