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

Revert "relax validation on the presence of the 'shouldRetry' field w… #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bbaldino
Copy link
Member

…hen there (#17)"

This reverts commit 725dcef.

DO NOT MERGE

This PR shouldn't be merged until we've cycled out all Jibris deployed before jitsi/jibri#232 is shipped. Once that is the case, we can merge this and enforce the presence of shouldRetry when a failure is reported.

@Neustradamus
Copy link

@bbaldino: What do you think about your PR now?

@bbaldino
Copy link
Member Author

I was just going through some old emails I hadn't gotten to and saw this, sorry for dropping the ball. I'm afraid I've forgotten most of the context, but do remember we did some enhancements on when to retry vs not for Jibri. @aaronkvanmeerten or @bgrozev
do you maybe remember anything around this?

@bgrozev
Copy link
Member

bgrozev commented Oct 18, 2022

I don't remember the reasons for this. The jibri's in use have definitely been updated by now, but it's hard to verify that no code ever sets failureReason without setting shouldRetry (but I trust you did that when you initially opened the PR)

@bbaldino
Copy link
Member Author

it's hard to verify that no code ever sets failureReason without setting shouldRetry (but I trust you did that when you initially opened the PR)

probably? :) but i've got the same concern. if things are working as-is, maybe it's best to leave it (i.e. not merge this)--at least not without someone doing some investigation to confirm.

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