-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
store: copy old value when refresh + cas #5683
Conversation
@@ -298,6 +298,10 @@ func (s *store) CompareAndSwap(nodePath string, prevValue string, prevIndex uint | |||
return nil, err | |||
} | |||
|
|||
if expireOpts.Refresh { |
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.
should this return an error if value != "" in case someone expects this to use the given value
like a refreshless CAS?
lgtm |
Hi @xiang90, I see this is labelled for backport. Which versions is this fix targeted for? |
@LLParse this was backported into 2.3.7. |
@heyitsanthony Thanks... For reference, I downloaded |
My code must be the problem. Sorry. It occurs to me that my initial write could've been erroneous when I ran into this issue and I simply ran into this issue when looking for an answer. Alternatively, I'm using the API incorrectly. This is the initial registration call. const (
nodeLockTTL = 30 * time.Second
)
n.Id = uuid.NewV4().String()
n.Info = &NodeInfo{n.Config.Hostname}
nodeInfoJson, _ := json.Marshal(n.Info)
n.InfoJson = string(nodeInfoJson)
log.Printf("Creating node %s\n", n.Id)
_, err := n.Keys.Set(
context.Background(),
fmt.Sprintf("/%s/nodes/%s", n.Config.ClusterName, n.Id),
n.InfoJson,
&etcd.SetOptions{TTL: nodeLockTTL}) This is the call that I perceived as setting the value to an empty string. _, err := n.Keys.Set(
context.Background(),
fmt.Sprintf("/%s/nodes/%s", n.Config.ClusterName, n.Id),
"",
&etcd.SetOptions{TTL: nodeLockTTL, Refresh: true}) |
Whoooops, definitely my mistake. It is functioning properly for me now - I must've messed up the initial set. My apologies, please disregard this. |
Fix #5651
/cc @heyitsanthony @mwitkow