search

jitsi / jitsi-xmpp-extensions

Common library holding all jitsi specific smack xmpp extensions.
Apache License 2.0
15 stars 51 forks source link

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

Open bbaldino opened 4 years ago

bbaldino commented 4 years ago

…hen there (#17)"

This reverts commit 725dcefc025a7aff22aaee6f152f0477b95a3947.

DO NOT MERGE

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

Neustradamus commented 2 years ago

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

bbaldino commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.