numaproj / numaflow-go

Numaflow Golang SDK
Apache License 2.0
46 stars 11 forks source link

server Start to return error instead of calling log.Fatal() #35

Closed sandangel closed 1 year ago

sandangel commented 1 year ago

In golang app, usually we handle the error at top level command. Sub function calls just return the error to the upper function for it to decide what to do with the error.

Can Numaflow UDF grpc server start method return the error instead of calling log.FatalF for error handling?

Example usage with Cobra

var myCmd = &cobra.Command{
    Use:          "my-cmd",
    RunE:         runQuery,
}

func init() {
    RootCmd.AddCommand(myCmd)
}

func handle(ctx context.Context, key string, d fn.Datum) fn.Messages {
    msg := d.Value()
    // If msg is not an integer, drop it, otherwise return it with "even" or "odd" key.
    if num, err := strconv.Atoi(string(msg)); err != nil {
        return fn.MessagesBuilder().Append(fn.MessageToDrop())
    } else if num%2 == 0 {
        return fn.MessagesBuilder().Append(fn.MessageTo("even", msg))
    } else {
        return fn.MessagesBuilder().Append(fn.MessageTo("odd", msg))
    }
}

func runQuery(cmd *cobra.Command, args []string) error {
    return server.New().RegisterMapper(fn.MapFunc(handle)).Start(context.Background())
}
whynowy commented 1 year ago

If the request is just for cobra.Command, use Run instead of RunE, which does not need to return an error.

The Start function in the SDK is intended to be the top level - it is used to start a gRPC server, it does not need to have an upper level to manage it - imagine with a gRPC server start error thrown to upper level, what you can do other than printing the log and letting the container panic? The SDK is intended to simplify the UDF implementation, returning an error means the SDK users have to deal with the error.

sandangel commented 1 year ago

@whynowy I still think it's better to let the user handle the error by themselves. They can ignore it if not care. But more robust applications have different ways of handling errors, like sending notifications and tracking errors in Sentry. If the sdk log.Fatalf then the app won't be able to handle such logic.

sandangel commented 1 year ago

Another case is where the user want to abstract numaflow away from the application code. They might be using multiple orchestration tool and only some functions will be handled by numaflow. They would create an interface and wrap around the numaflow-sdk. Not returning error won't let them handle the errors nicely

sandangel commented 1 year ago

If you think it's better to not have user handling the error, I can create a separated function, like StartE and that one will return error. Does it sound good?

whynowy commented 1 year ago

Makes sense, thanks!