Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

Implement error types for functions #423

Merged
merged 2 commits into from
May 15, 2018
Merged

Conversation

rjew
Copy link
Contributor

@rjew rjew commented May 9, 2018

Fixes #356, Fixes #403

  • Implement error handling spec
  • Distinguish between error types: InputError, FunctionError, SystemError
  • Displays the error and its properties (type, stacktrace, etc.) when a function invocation encounters an error
  • Added unit tests and a small e2e test
  • Requires some changes in the language packs

Copy link
Contributor

@berndtj berndtj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Lots of new tests! I just want to remove "transient" :)

type: array
items:
type: string
transient:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to @imikushin about this property. I'd like us to remove it as it's just going to be confusing and REALLY hard to get right.

Copy link
Contributor

@imikushin imikushin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes. Thanks for making this happen!

@@ -832,7 +832,7 @@ definitions:
type: integer
message:
type: string
user-error:
input-error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove 'input-error' and 'function-error' fields from event-manager API: by checking their generated code usages, I see they're not used anywhere.

Also, they break our field naming convention (camelCase).


run.Error = ctx.GetError()
if run.Error != nil {
return errors.Wrapf(&invocationError{run.Error}, "error running function: %s", run.FunctionName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error stacktrace seems to be getting lost while recording the result of the function invocation

}
var out functions.Message
if err := json.Unmarshal(resBytes, &out); err != nil {
return nil, errors.Errorf("cannot JSON-parse result from OpenFaaS: %s %s", err, string(resBytes))
return nil, &outputError{errors.Errorf("cannot JSON-parse result from OpenFaaS: %s %s", err, string(resBytes))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a system error too: it's the function runtime responsible for JSON-encoding the response, not the function.

}

var out functions.Message
if err := json.Unmarshal(resBytes, &out); err != nil {
return nil, errors.Errorf("cannot JSON-parse result from riff: %s %s", err, string(resBytes))
return nil, &outputError{errors.Errorf("cannot JSON-parse result from riff: %s %s", err, string(resBytes))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a system error too: it's the function runtime responsible for JSON-encoding the response, not the function.

@rjew rjew force-pushed the function-error branch 2 times, most recently from 82d3a3e to 331209e Compare May 11, 2018 16:26
berndtj
berndtj previously approved these changes May 15, 2018
@berndtj
Copy link
Contributor

berndtj commented May 15, 2018

@rjew can you rebase after the big model refactor

@rjew
Copy link
Contributor Author

rjew commented May 15, 2018

Sure working on it now

Implement error handling types: InputError, FunctionError, SystemError.
@rjew rjew merged commit 68cacd9 into vmware-archive:master May 15, 2018
@rjew rjew deleted the function-error branch May 15, 2018 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants