Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

js: Add JetStreamError with more API response details #1047

Merged
merged 7 commits into from Aug 23, 2022
Merged

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Aug 12, 2022

Follow up from #1044

This adds a JetStreamError interface that exposes from details about the API response when it is one:

// JetStreamError is an error result that happens when using JetStream.
type JetStreamError interface {
	APIError() *APIError
	error
}

To keep it backwards compatible and keep old checks working, instead of defining JetStream errors like this:

ErrJetStreamNotEnabled = errors.New("nats: jetstream not enabled")

They can be defined like the following:

ErrJetStreamNotEnabled JetStreamError = &jsError{apiErr: &APIError{ErrorCode: JSErrCodeJetStreamNotEnabled, Description: "jetstream not enabled"}}

Signed-off-by: Waldemar Quevedo wally@nats.io

@wallyqs wallyqs force-pushed the js-api-err branch 2 times, most recently from 3d28ca3 to 2540923 Compare August 12, 2022 23:22
@coveralls
Copy link

coveralls commented Aug 12, 2022

Coverage Status

Coverage decreased (-0.2%) to 85.638% when pulling f7eaca6 on js-api-err into b81c9e7 on main.

jsm.go Outdated
Code() int
ErrorCode() int
Description() string
Error() string
Copy link
Member Author

Choose a reason for hiding this comment

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

could add some other conditions to be able signal the client that something is Temporary() here too, for example when there are transient errors such as JSClusterNotAvailable

@wallyqs
Copy link
Member Author

wallyqs commented Aug 14, 2022

Another idea is to make a more general JetStreamError but that can still unwrap the APIError: https://github.com/nats-io/nats.go/compare/js-api-err2?expand=1

@piotrpio
Copy link
Collaborator

I think I like the second approach (form js-api-err2 branch) a little bit more. It is a bit easier to distinguish API error from client JetStream error if the API error is stored on 1 struct field, not mixed together with message. Plus, it makes defining jsError from APIError more concise.

Signed-off-by: Waldemar Quevedo <wally@nats.io>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
… type

Signed-off-by: Waldemar Quevedo <wally@nats.io>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
Signed-off-by: Waldemar Quevedo <wally@nats.io>
@wallyqs wallyqs marked this pull request as ready for review August 22, 2022 21:44
@piotrpio
Copy link
Collaborator

  1. Converted all JetStream-related errors to jsError
  2. Switch from comparing error codes to errors.Is() in JetStream methods
  3. Moved errors to a separate file
  4. Removed several unused errors

return fmt.Sprintf("nats: %s", err.message)
}

func (err *jsError) Unwrap() error {
Copy link
Member Author

@wallyqs wallyqs Aug 23, 2022

Choose a reason for hiding this comment

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

More info about errors package features of wrapping and Unwrap can be found here:

@piotrpio piotrpio removed their request for review August 23, 2022 16:56
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@wallyqs wallyqs merged commit 164805c into main Aug 23, 2022
@wallyqs wallyqs deleted the js-api-err branch August 23, 2022 17:17
@wallyqs wallyqs changed the title js: Add JetStreamAPIError with more api response details js: Add JetStreamError with more API response details Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants