search

lightsparkdev / go-sdk

Lightspark Go SDK
Apache License 2.0
5 stars 6 forks source link

Add RequestError and GraphQLError types to indicate server failure and graphql failure. #95

Closed zhenlu closed 5 months ago

zhenlu commented 5 months ago

Fixes LIG-5257

zhenlu commented 5 months ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @zhenlu and the rest of your teammates on Graphite Graphite

zhenlu commented 5 months ago

I think the RequestError will be the things for them to retry. GraphQLError is the error returned from graphql. All other errors are the errors with their code.

I don't think they should retry on GraphQLError here but it did indicate either side have a bug in code that needs to be fixed.

mgorven commented 5 months ago

@mgorven can correct me if I'm wrong, but I'm not sure if this totally solves what they're looking for. The most important piece is being able to tell "is this my fault or lightspark's fault and should I retry". @mgorven is it true that anything with an extension is likely to be a non-retryable user error?

It's mostly but not completely true. There are a few cases where we have an extension but it's not necessarily the user's fault (exception classes here which inherit directly from ExternalException, currently NoRoutes and CardPaymentError).

I don't think they should retry on GraphQLError here but it did indicate either side have a bug in code that needs to be fixed.

In principal you're right, but in practice I think customers might want to. e.g. if sparkcore fails to communicate with sparknode because it's restarting that would surface as a graphql error (with no error_name).

zhenlu commented 5 months ago

So basically: RequestError: should retry GraphQLError with empty Type: could retry Other GraphQLError: don't retry.

@mgorven do you think they can retry on NoRoutes and CardPaymentError? If so I think we need to add a boolean field in the GraphQLError to indicate which one can be retried.

mgorven commented 5 months ago

@mgorven do you think they can retry on NoRoutes and CardPaymentError? If so I think we need to add a boolean field in the GraphQLError to indicate which one can be retried.

CardPaymentError is only used for billing, so not relevant for the SDK. I don't think that retrying NoRoute is going to help (if routefinder can't find a route it's not going to have one a few seconds later).

zhenlu commented 5 months ago

I can probably break the GraphQLError into two separate ones so one can retry while the other cannot.

zhenlu commented 5 months ago

RequestError: should retry GraphQLInternalError: could retry GraphQLError: don't retry

jklein24 commented 5 months ago

RequestError: should retry GraphQLInternalError: could retry GraphQLError: don't retry

Can we document (in code) the error structs to this effect so that callers can see what to do with each type?

zhenlu commented 5 months ago

Merge activity