Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
state: Add support for sequences with a minimum #6839
Conversation
| + } | ||
| + if curVal == 0 { | ||
| + // No sequence document exists, create one. | ||
| + ok, err := sequence.create(minVal + 1) |
howbazaar
Jan 19, 2017
Owner
Am I right in remembering that the sequence values stores the next value of the sequence?
| + // No sequence document exists, create one. | ||
| + ok, err := sequence.create(minVal + 1) | ||
| + if err != nil { | ||
| + return -1, errors.Annotate(err, "could not create sequence") |
howbazaar
Jan 19, 2017
Owner
Is this really an error? Why not a retry?
Perhaps recording the semantics of the create method in the interface.
| + bson.M{"$set": bson.M{"counter": next}}, | ||
| + ) | ||
| + if errors.Cause(err) == mgo.ErrNotFound { | ||
| + return false, nil |
mjs
Jan 24, 2017
Contributor
The update query includes the expected current value of the sequence. The update will fail if ErrNotFound if someone update the sequence, allowing detection of concurrent updates.
| + for i := 0; i < iterations; i++ { | ||
| + v, err := nextSeq() | ||
| + if err != nil { | ||
| + fmt.Println(err) |
babbageclunk
approved these changes
Jan 24, 2017
This looks good to me. I was a bit confused about returning -1 rather than 0 on errors, but I guess that's just being extra careful that the caller won't think this is a valid value.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
mjs commentedJan 19, 2017
•
Edited 1 time
-
mjs
Jan 19, 2017
Added State.sequenceWithMin() and the infrastructure to support it. This
is like State.sequence() but allows a minimum value for a sequence to be
specified. This is surprisingly complicated to implement due to there
being no atomic way to provide initial values for an upserted counter in
MongoDB.
This is preparation for supporting tracking of which local charm URLs
have been used without having to keep dead charm documents.
See https://bugs.launchpad.net/juju/+bug/1657614
QA
This functionality isn't wired up yet but the unit tests, which include a live contention test, have been run under the stress tested on 2 machines for over an hour.