Following up on #138: this PR makes a few changes:
Changed UndoActionError (which was an enum with one variant) to a simpler struct representing only that case. I decided while working on this that the generality of an enum didn't seem worth it.
Changed the type signature for undo actions to return UndoActionPermanentError
The goal here is to make it more obvious to authors of undo action functions that it's rather a big deal to return an error from an undo action and they should only do that for permanent errors. This isn't a change in behavior.
This is the sort of change where we'll definitely want to convert important consumers before committing to this (i.e., landing this PR). I've started converting Omicron, and I'm honestly not sure if this is worthwhile. There are over 50 undo actions, and each may have a few code paths that need adjustments, and I'm not sure enough about this change to go do that.
Some design notes about this:
I think what's most important is that when you write the type signature, the type says that this is a permanent failure.
At one point I also thought it would be nice if consumers explicitly had to wrap any error they produced with UndoActionPermanentError, but as I started making such a change mechanically, it seemed like a waste of time to convert existing stuff.
As a result, I chose to impl From<anyhow::Error> for UndoActionPermanentError, figuring that any existing consumers are already producing anyhow::Error. This helps, but in practice, most of the undo actions in Omicron are not directly producing anyhow::Error, but rather producing omicron_common::api::external::Error, which impl's std::error::Error, for which anyhow::Error impls From. So in practice, quite a lot of call sites need to be updated anyway.
I considered impl'ing From<StdError> for UndoActionPermanentError, but this would preclude it from impl'ing StdError directly, which means we can't use thiserror and can't use it like many other kinds of errors. (This isn't a non-starter, though.)
Anything that directly produces an ActionError and then uses ? doesn't need to do anything because that can be converted to UndoActionPermanentError. This is convenient for the various helper functions that Steno provides that produce ActionError (like lookup()).
We'd want to check that this change doesn't break compatibility with previously-serialized saga logs (or else that we don't care about that).
There are plenty of bigger questions here, too, like: should Steno first-class the idea of retryable failures? I want to defer this discussion from this PR, unless we feel it invalidates this step. There are a lot of tricky questions around doing this. (What policy do we use? How does Steno tell whether an error is retryable or not? How does it report on transient failures?) I think we can best approach this by prototyping with a helper function (i.e., in Omicron) that we use to wrap undo actions. If we get to the point where we've got a stable abstraction that's a clear win and it makes sense to put it into Steno, then we can do it.
Following up on #138: this PR makes a few changes:
UndoActionError
(which was an enum with one variant) to a simpler struct representing only that case. I decided while working on this that the generality of an enum didn't seem worth it.UndoActionPermanentError
The goal here is to make it more obvious to authors of undo action functions that it's rather a big deal to return an error from an undo action and they should only do that for permanent errors. This isn't a change in behavior.
This is the sort of change where we'll definitely want to convert important consumers before committing to this (i.e., landing this PR). I've started converting Omicron, and I'm honestly not sure if this is worthwhile. There are over 50 undo actions, and each may have a few code paths that need adjustments, and I'm not sure enough about this change to go do that.
Some design notes about this:
impl From<anyhow::Error> for UndoActionPermanentError
, figuring that any existing consumers are already producinganyhow::Error
. This helps, but in practice, most of the undo actions in Omicron are not directly producinganyhow::Error
, but rather producingomicron_common::api::external::Error
, which impl'sstd::error::Error
, for whichanyhow::Error
implsFrom
. So in practice, quite a lot of call sites need to be updated anyway.From<StdError> for UndoActionPermanentError
, but this would preclude it from impl'ingStdError
directly, which means we can't usethiserror
and can't use it like many other kinds of errors. (This isn't a non-starter, though.)ActionError
and then uses?
doesn't need to do anything because that can be converted toUndoActionPermanentError
. This is convenient for the various helper functions that Steno provides that produceActionError
(likelookup()
).We'd want to check that this change doesn't break compatibility with previously-serialized saga logs (or else that we don't care about that).
There are plenty of bigger questions here, too, like: should Steno first-class the idea of retryable failures? I want to defer this discussion from this PR, unless we feel it invalidates this step. There are a lot of tricky questions around doing this. (What policy do we use? How does Steno tell whether an error is retryable or not? How does it report on transient failures?) I think we can best approach this by prototyping with a helper function (i.e., in Omicron) that we use to wrap undo actions. If we get to the point where we've got a stable abstraction that's a clear win and it makes sense to put it into Steno, then we can do it.