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

Ease interpretation of errors that arise during registration operations #45

Closed
seh opened this issue Aug 31, 2016 · 2 comments
Closed

Comments

@seh
Copy link
Contributor

seh commented Aug 31, 2016

At present, the following functions return type error:

They can fail for many reasons, but a few stand out:

  • DeregisterInstance fails if the instance does not exist (HTTP status code 404).
  • DeregisterInstance fails if the instance did exist (HTTP status code 200).
    This is due to expecting status code 204, even though the Eureka documentation promises status code 200 for a successful deletion. This mismatch makes me wonder if anyone else is using this method.

To work around these problems, I had to write the following function to filter errors that arise when calling DeregisterInstance:

func deregistrationFailureIsInnocuous(err error) bool {
    msg := err.Error()
    return strings.Contains(msg, "200") || strings.Contains(msg, "404")
}

That is clear evidence of a gap in the interface.

Both ReregisterInstance and RegisterInstance can fail with status code 400 if Eureka deems the instance payload invalid. Note that it's not possible for registration to fail due to a collision with an existing instance with the same ID; Eureka replaces the existing registration.

It would be helpful for callers to be able to discern why these operations failed. Rather than introducing sentinel error values or types, I propose that we introduce a few exported functions:

  • InstanceWasMissing(error) bool
    True for failures in DeregisterInstance due to receiving status code 404
  • InstanceWasInvalid(error) bool
    True for failures in RegisterInstance and DeregisterInstance due to receiving status code 400

Do you have counter-proposals for these names? Are there other functions you can think of that would clarify what went wrong?

@seh
Copy link
Contributor Author

seh commented Aug 31, 2016

I noticed that fargo already exports type AppNotFoundError. I'd rather not repeat that technique here.

@seh
Copy link
Contributor Author

seh commented Sep 28, 2016

See this gist (errors.go) for an adaptation of the interpretive predicate functions I had proposed in #47. You can see that the caller still has consider which operation he had attempted in order to know which function to call, due to Eureka's irregular treatment of metadata updates, but it beats parsing the error messages.

If you change your mind about whether fargo should include these or similar predicates, we can open another issue.

@seh seh closed this as completed Sep 28, 2016
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

No branches or pull requests

1 participant