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

Add ErrKeyExists on kv.Create #1135

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Add ErrKeyExists on kv.Create #1135

merged 1 commit into from
Nov 16, 2022

Conversation

bruth
Copy link
Member

@bruth bruth commented Nov 16, 2022

Fix #1134

Signed-off-by: Byron Ruth <b@devel.io>
// Check if the expected last subject sequence is not zero which implies
// the key already exists.
if aerr, ok := err.(*APIError); ok {
if aerr.ErrorCode == 10071 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if there is a better way to identify this particular error type.

Copy link
Member

Choose a reason for hiding this comment

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

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.599% when pulling 7230ca7 on issue-1134 into 1cc4427 on main.

// Check if the expected last subject sequence is not zero which implies
// the key already exists.
if aerr, ok := err.(*APIError); ok {
if aerr.ErrorCode == 10071 {
Copy link
Member

Choose a reason for hiding this comment

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

something that we started doing in the nats.go client is to move these codes into jserrors.go:
https://github.com/nats-io/nats.go/blob/main/jserrors.go#L121

then to make it compatible with errors.Is defining something like:

ErrWrongLastSequence JetStreamError = &jsError{apiErr: &APIError{ErrorCode: JSErrStreamWrongLastSequence}}

then this check would become:

if errors.Is(err, ErrWrongLastSequence) {
 return ErrKeyExists
}

What I think might be helpful eventually is to have something like another KeyValueError type that embeds the original JS APIError to be able to inspect the original JS API Error which resulted into the KeyValueError by using errors.As.

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments on improving the way we represent errors from KeyValue API but can be done later.

@bruth
Copy link
Member Author

bruth commented Nov 16, 2022

Thanks @wallyqs I can make a subsequent PR for that. I don't have access to merge, so feel free.

@wallyqs wallyqs merged commit 5044b6e into main Nov 16, 2022
@wallyqs wallyqs deleted the issue-1134 branch November 17, 2022 04:01
@ripienaar
Copy link
Contributor

Is this is a API breaking change @wallyqs @bruth ?

@bruth
Copy link
Member Author

bruth commented Nov 17, 2022

@ripienaar Good call out.. technically yes given that it changes how a person would need to check for an error and the existing manual method of checking for the code would now not work. A user on Slack called it out since the error not intuitive without understanding the implementation, so it debatable if we could frame it as such.

All that said, how has this been handled in the past? Should we defer all error improvements to a separate branch and then bump the major version?

@ripienaar
Copy link
Contributor

The way the coded errors are handled now is really awkard as you mentioned, we could have something like nats.IsNatsErr(err, 1234) to check the error code as well as standard go wrapping checks in there, all the casting and IsError etc we do now is super verbose.

I don't know the anwer but I am generally concerned about backward compatibility so that was my main concern.

In this case, obviously your commit is an improvement and one could even argue surfacing a underlying nats error in the kv api is just a bug.

@wallyqs
Copy link
Member

wallyqs commented Nov 17, 2022

that's a good point, to avoid being a breaking change we could go back to returning the raw APIError (return err) and instead define ErrKeyExists with the embedded APIError something like this:

ErrKeyExists JetStreamError = &jsError{apiErr: &APIError{ErrorCode: JSErrStreamWrongLastSequence, Code: 400}}

then that way those checking with errors.Is(err, ErrKeyExists) would be able to handle with the new behavior and previous usage with the raw APIError type would still work without changes.

@bruth
Copy link
Member Author

bruth commented Nov 17, 2022

argue surfacing a underlying nats error in the kv api is just a bug.

Yea, that is sort of how it felt and I think would be acceptable framing in this case..

then that way those checking with errors.Is(err, ErrKeyExists) would be able to handle with the new behavior and previous usage

I think wrapping is a good idea for the existing set of ErrXX values in order to migrate everything over to jserrors.go and expecting users to use errors.Is going forward. Looking at the distinction of the ApiError vs. JetStreamError, where does this distinction get used in practice? Likewise, if we were to introduce a KVError, I am not sure if having different types are useful compared to have different named values, e.g. ErrKeyValueKeyExists (following the ErrJetStream... naming convention). Just thinking out loud..

@ripienaar
Copy link
Contributor

We liked the idea of the error codes in the JetStream errors (see nats error ls for example as a great source of information that we could lean into, links to doc site etc).

So we started with JetStream work as a place to bake the error system and discover what work and doesn't, it went through a few iterations before we ended up where we are now.

I'd personally like to see this now spread out to other server errors and into clients with deeper awareness of the metadata that goes with the errors etc

For KV though, surfacing a coded error is almost always a bug imo, KV API is small and easy to understand and focussed on one thing. It's errors should be a constrained enough set that you can have constants defined for them like the current not found ones and that those will remain pretty barebones to not end up painting us into corners when it comes to supporting other storage backends which seems inevitable.

$ nats errors lookup 10121
NATS Error Code: 10121

        Description: consumer max ack pending exceeds system limit of {limit}
          HTTP Code: 400
  Go Index Constant: JSConsumerMaxPendingAckExcessErrF

No further information available

@bruth
Copy link
Member Author

bruth commented Nov 17, 2022

We liked the idea of the error codes in the JetStream errors [...] I'd personally like to see this now spread out to other server errors and into clients with deeper awareness of the metadata that goes with the errors etc

Yes I definitely agree with that, richer errors with more metadata the better and we definitely want to lean into a source of truth of errors for docs and and clients to stay consistent.

My question was more around what value the JetStreamError type provides since it wraps ApiError.

@ripienaar
Copy link
Contributor

Others might be better to comment there - but since they are essentially only produced by JetStream today, and we dont know how it would look for non JS errors - seperating them might be a way to improve long term api stability should the errors in non JS uses from the server deviate a bit (we dont always have like HTTP status error codes etc)

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.

Add error value for when a KV key already exists
4 participants