Implement onFailure handler.
#63
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KiKoS0
force-pushed
the
riadh/inn-3326-implement-on-failure
branch
from
fed7cba
to
4a366ed
Compare
KiKoS0
requested review from
djfarrelly,
darwin67,
jpwilliams and
tonyhb
as code owners
KiKoS0
force-pushed
the
riadh/inn-3326-implement-on-failure
branch
2 times, most recently
from
ed938dc
to
24d874a
Compare
albertchae
approved these changes
Sep 11, 2024
There was a problem hiding this comment.
LGTM, probably fine to merge now per discussion on https://linear.app/inngest/issue/INN-3551/document-how-we-should-implement and we can iterate on any feedback after cc: @djfarrelly
| private val functions = | ||
| functions | ||
| .mapValues { (_, fn) -> fn.toInngestFunction() } | ||
| .plus(generateFailureFunctions(functions, client)) |
There was a problem hiding this comment.
nit: slightly confusing that functions being passed to generateFailureFunctions is referring to the param, maybe worth creating a separate local variable for failureFunctions that you then call with plus? or rename this local val to inngestFunctions?
Comment on lines
+46
to
+55
| Optional<EventEntry> event = Arrays.stream(devServer.listEvents().getData()) | ||
| .filter(e -> "inngest/function.failed".equals(e.getName()) && eventName.equals(e.getData().getEvent().getName())) | ||
| .findFirst(); | ||
|
|
||
| assert event.isPresent(); | ||
|
|
||
| RunEntry<Object> onFailureRun = devServer.runsByEvent(event.get().getInternal_id()).first(); | ||
|
|
||
| assertEquals("Completed", onFailureRun.getStatus()); | ||
| assertEquals("On Failure Success", (String) onFailureRun.getOutput()); |
Comment on lines
33
to
36
| val onFailureMethod = this.javaClass.getMethod("onFailure", FunctionContext::class.java, Step::class.java) | ||
|
|
||
| if (onFailureMethod.declaringClass != InngestFunction::class.java) { |
There was a problem hiding this comment.
nice! maybe add a short comment here like only generate failure handler if onFailure was overridden?
| @@ -9,7 +9,7 @@ import java.time.Duration | |||
| // TODO: Throw illegal argument exception | |||
| class InngestFunctionConfigBuilder { | |||
| var id: String? = null | |||
| private var name: String? = null | |||
| internal var name: String? = null | |||
There was a problem hiding this comment.
this is because you use it to build the function name for the failure function, right?
I took a more OOP approach for this feature which is slightly different
than the other SDKs but we can discuss this further in the PR review.
The `WithFailureHandler` interface has a single method `onFailure` that
is called when the function fails. An example of how to use this:
```kotlin
class MyFunction : InngestFunction(), WithFailureHandler {
override fun execute(ctx: FunctionContext, step: Step): Any? {
// Do something
}
override fun onFailure(ctx: FunctionContext, step: Step): Any? {
// Handle failure
}
}
```
This removes the `WithFailureHandler` interface and instead provides an
overridable `onFailure` method in `InngestFunction` that can be used to
define a function to be called when the function fails.
It's used in the same way but without the need to implement an interface
```kotlin
class MyFunction : InngestFunction() {
override fun execute(ctx: FunctionContext, step: Step): Any? {
// Do something
}
override fun onFailure(ctx: FunctionContext, step: Step): Any? {
// Handle failure
}
}
```
KiKoS0
force-pushed
the
riadh/inn-3326-implement-on-failure
branch
from
24d874a
to
d8d9b23
Compare
|
Merging this now and we'll address feedback in follow-ups. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
TheWithFailureHandlerinterface has a single methodonFailurethatis called when the Inngest function fails.
Following this discussion we went with the 4th approach of having an
optionally overridable
onFailuremethod inInngestFunction. Andconditionally check if it's overridden through reflection and register it.
https://stackoverflow.com/questions/2315445/how-to-quickly-determine-if-a-method-is-overridden-in-java/2315467#2315467
An example of how to use it in Kotlin:
Checklist
Related