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

Enable Multi-site mode features. #758

Merged
merged 28 commits into from
Feb 7, 2023

Conversation

tadscottsmith
Copy link
Contributor

There are a number of people that have been testing this and hoping to see it in the master branch. I am open to any suggestions if there's something you'd like to see done a different way. This shouldn't impact anyone unless they enable the new configuration options.

@robotastic
Copy link
Owner

This is awesome! I am going to have to spend some time really looking through it - but it sounds like a lot of folks have gotten it to work well.

@tadscottsmith
Copy link
Contributor Author

Great! Let me know if you have any suggestions or want me to update anything. This version primarily helps conserve recorder resources by only recording the call from one site. I also think it would be worthwhile to have an option to record the same call from both sites, and then use some logic in the call concluder to take the best transmissions from either site to attempt to create the best quality call.

One big thing I'd like your input on is tracking the reason that calls are in a monitoring state. I initially implemented a separate MonitoringState struct (as linked below), but I'm curious if you'd prefer to have everything in the original "State" or if you're onboard with breaking out the additional monitoring states? A few future use cases I could see are starting a monitored call if a recorder becomes available mid call, or creating metadata files / plugin calls for certain types of monitored calls that someone doesn't chose to record. https://github.com/tadscottsmith/trunk-recorder/blob/88b4289d0713d796411eeb1ccdb9e5f0d96600a2/trunk-recorder/state.h#L13-L20

@tadscottsmith
Copy link
Contributor Author

Let me know if it would be helpful to break this up into multiple PRs. I could do one to add monitoring state, and a separate one to add the optional multi-site config.

@robotastic
Copy link
Owner

robotastic commented Feb 7, 2023 via email

@robotastic robotastic merged commit 47c27ca into robotastic:master Feb 7, 2023
@robotastic
Copy link
Owner

This looks great! I think Monitoring state should help out things in general. And it should work pretty much out of the box for most folks, and you can make it work better with a little configuration

@tadscottsmith tadscottsmith deleted the multi-site-v1 branch May 23, 2023 21:49
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