-
Notifications
You must be signed in to change notification settings - Fork 75
Playing around with state consolidation #32
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
Conversation
for _, members := range cluster.Members { | ||
if members.ID == id { | ||
return nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting state that I haven't fully though about is the case where a member is registered, but for whatever reason the hostname
, region
, org primary
fields might be different. I don't think this should be possible though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably safe to assume they will be different. If they aren't, something is broken with Machines.
if errors.Is(err, ErrCAS) { | ||
c.RegisterMember(id, hostname, region, primary) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible infinite loop in the case of consul misbehaving? feels unlikely though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We don't want to be spamming Consul if it's under load. I'll see about addressing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this should be fairly safe. We will only retry in the event of a CAS error, which should be super rare on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will only retry in the event of a CAS error, which should be super rare on its own.
ah cool yeah. I'm not familiar with consul failure modes so I wasn't sure if a degraded cluster can cause CAS errors or something. Seems like no.
b47d36e
to
d51f57c
Compare
Few notable changes: