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

Garbage collector for active calls #912

Merged
merged 12 commits into from
May 30, 2022
Merged

Conversation

aeatencio
Copy link
Contributor

@aeatencio aeatencio commented May 24, 2022

When a call remains active for too long Verboice considers there was an error and cancels it.

This GC runs every X minutes and cancels every active call for more than N minutes.

The value X defaults to 10 minutes and is configurable via the ENV variable minutes_between_active_calls_gc_runs.

The value N defaults to 120 minutes and is configurable via the ENV variable minutes_for_cancelling_active_calls.

For #900

Bonus track: buggy code deletion.

@aeatencio aeatencio requested review from ysbaddaden and ggiraldez May 24, 2022 13:43
@aeatencio aeatencio self-assigned this May 24, 2022
Copy link
Member

@matiasgarciaisaia matiasgarciaisaia left a comment

Choose a reason for hiding this comment

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

Do we know if the broker understands that a call got cancelled with respect to the channel limit? ie, will it start a new call if any one is queued?

I'm also thinking about the callbacks - will they trigger? My fear is that we're silently just marking calls as failed, without that affecting the behaviour of the system.

@aeatencio
Copy link
Contributor Author

Do we know if the broker understands that a call got cancelled with respect to the channel limit? ie, will it start a new call if any one is queued?

@matiasgarciaisaia, you may be right! Thanks!!!

This is what we do in the API. Maybe we need to include queued_calls this way too, right?

I'm also thinking about the callbacks - will they trigger? My fear is that we're silently just marking calls as failed, without that affecting the behavior of the system.

No clue about how to check this. But what I think is that we're reproducing how the calls are being canceled manually, so it should work, right?

aeatencio added 2 commits May 24, 2022 16:16
Cancelling a queued_call just means:
updating its call_log state to "cancelled"
aeatencio added 3 commits May 24, 2022 17:05
This reverts commit a88b737.
Cancelling a queued_call just means:
updating its call_log state to "cancelled"
@aeatencio
Copy link
Contributor Author

@matiasgarciaisaia, please let me know how you see these changes now. I know this may not be the ideal solution, but please let me know if you think it's good enough. Thanks!!

Copy link
Member

@matiasgarciaisaia matiasgarciaisaia left a comment

Choose a reason for hiding this comment

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

Let's change the timer:send_interval thing and merge 👍

We can leave the changes in the GC start time for later.

aeatencio pushed a commit that referenced this pull request May 26, 2022
aeatencio pushed a commit that referenced this pull request May 26, 2022
@aeatencio
Copy link
Contributor Author

I think we finished with this, @matiasgarciaisaia. Waiting for the CI to finish and your approval to merge! Thanks a lot!

@aeatencio aeatencio force-pushed the fix/900-active-calls-gc branch from 8e1ffa8 to 7260d80 Compare May 27, 2022 19:47
@aeatencio
Copy link
Contributor Author

Finally, @matiasgarciaisaia, I think we're done. This is the line I added to avoid the CI crashing 🙂

@aeatencio aeatencio merged commit 0d5c136 into master May 30, 2022
@aeatencio aeatencio deleted the fix/900-active-calls-gc branch May 30, 2022 12:41
@aeatencio
Copy link
Contributor Author

@matiasgarciaisaia, I've just merged this, but could I ask for your approval on this PR? 🙏 Thanks!!

@matiasgarciaisaia
Copy link
Member

I've reviewed the PR on Friday but forgot to comment. All is 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.

2 participants