livepeer / go-livepeer

Official Go implementation of the Livepeer protocol
http://livepeer.org
MIT License
538 stars 169 forks source link

P1 - Return a 422 when B hits max # of transcode attempts #2675

Closed yondonfu closed 1 year ago

yondonfu commented 1 year ago

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

The HandlePush handler currently returns a 422 error to the client if a non-retryable error is encountered.

This 422 is important to MistProcLivepeer for a few reasons here:

Describe the solution you'd like A clear and concise description of what you want to happen.

I think it could be worthwhile to return a 422 for maxing out transcode attempts as well to trigger the above conditions in MPL.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

thomshutt commented 1 year ago

@mjh1 this is another good candidate for you to take a look at

mjh1 commented 1 year ago

@yondonfu @thomshutt It looks like this has already been implemented: https://github.com/livepeer/go-livepeer/blob/fa3564565e7aa7d40be14502b8a217c7a541b740/server/broadcast.go#L1557

Also confirmed with a break point that there's a unit test which covers this case: https://github.com/livepeer/go-livepeer/blob/4be57ce2328c9d3a8fdfcfdd8354f5c675f4b361/server/push_test.go#L111-L116

What do you think?

mjh1 commented 1 year ago

@cyberj0g Would you mind checking my above question and verifying what I found? Looks fairly simple

cyberj0g commented 1 year ago

Right, it seem to be already returning 422 when max transcode attempts reached. My only concern is hacky substring matching inside isNonRetryableError to determine that. Could we try to fix this? There are many other spots, where we can't use the constant of error type, because we need to customize the message, and ending up with multiple similar string constants scattered across the code.

yondonfu commented 1 year ago

@mjh1

It looks like this has already been implemented

Good catch! It does look like the push handler is returning a 422 if the max # of transcode attempts is hit.

mjh1 commented 1 year ago

@cyberj0g yep that isn't the nicest. what do you think about using a custom error type instead: https://github.com/livepeer/go-livepeer/pull/2729

mjh1 commented 1 year ago

@cyberj0g could you check my PR? Hopefully I understood you right.