Prevent Rust transform death spiral by not panicking on transform returning Err
#18530
Conversation
If a Rust transform returns Err, the runtime panics because it currently calls expect on the Result. This makes any Err returned by a users transform put the wasm runtime into a death spiral of panic and restart loops. Until the runtime has a concept of handling "bad" records that cause errors (e.g. DLQ, drop, etc.) the transform should be responsible for this. Returning Err should just result in a log message, at worst.
voutilad
changed the title
Err
| .inspect_err(|e| eprintln!("transforming record failed with error: {:?}", e)) | ||
| .unwrap_or(()) |
There was a problem hiding this comment.
Looks good, and thanks for the change. I'm hesitant to merge right away since the panic-on-expect behavior feels intentional, and it may be directly related to the transform execution model viz how we calculate backoffs and such. Just need to give it some though.
There was a problem hiding this comment.
This will drop records that fail on the floor, which is not a safe default because there is no way to know what failed where. I don't agree with this change (of course my opinion doesn't matter anymore), users should opt into this. For consistency other SDKs should change to this (go sdk panics on failures).
This default pauses until a deploy happens and the developers fix the code.
There was a problem hiding this comment.
@rockwotj I don’t see the pause behavior you describe. What I see is the same record being attempted to be reprocessed repeatedly, causing the same panic over and over again spamming the logs.
If the behavior is to panic, there’s also no reason to use a Result type. Just have the transform code panic directly. Returning Err just to cause a panic is very surprising as someone writing code against the api.
There was a problem hiding this comment.
Yes there are retred because we can't tell the difference between an OOM where a retry can fix and a permanent failure right now. That is fixable without SDK changes (basically if the user code fails as the first batch it will never succeed)
RE API - it's future proofing it. We should have an SDK or yanl configurable way to set the dead letter queue, then we can do the dead letter queue pattern automatically for a user.
If a Rust transform returns Err, the runtime panics because it currently calls expect on the Result. This makes any Err returned by a users transform put the wasm runtime into a death spiral of panic and restart loops. Until the runtime has a concept of handling "bad" records that cause errors (e.g. DLQ, drop, etc.) the transform should be responsible for this. Returning Err should just result in a log message, at worst.
Backports Required
Release Notes
Errwhen processing a message.Bug Fixes
Err.