Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Use sequences to track used charm URLs #6866
Conversation
| @@ -183,3 +189,25 @@ func (su *dbSeqUpdater) set(expected, next int) (bool, error) { | ||
| } | ||
| return true, nil | ||
| } | ||
| + | ||
| +func (su *dbSeqUpdater) ensure(next int) error { |
| + ok, err = su.set(curVal, next) | ||
| + } | ||
| + if !ok { | ||
| + return errors.New("unexpected contention") |
wallyworld
Jan 25, 2017
Owner
Elsewhere it says "too much contention while updating sequence"
Can we make this a little verboser too?
| @@ -57,13 +56,9 @@ func (st *State) sequence(name string) (int, error) { | ||
| // `sequence` is more efficient than `sequenceWithMin` and should be | ||
| // preferred if there is no minimum value requirement. | ||
| func (st *State) sequenceWithMin(name string, minVal int) (int, error) { | ||
| - sequences, closer := st.getCollection(sequenceC) | ||
| + sequences, closer := st.getRawCollection(sequenceC) |
wallyworld
Jan 25, 2017
Owner
This change implies a gap in unit test coverage?
Do there need to be multi-model tests added?
mjs
Jan 25, 2017
Contributor
The existing tests already check multi-model behaviour. This is an internal only change.
| - // Note that this aligns with the existing contract implied by | ||
| - // Dead: that most clients should see it as not existing at all. | ||
| - // Nothing strictly obliges us to clean up the doc. | ||
| + // immortal. |
wallyworld
Jan 25, 2017
Owner
There should be changes to the charm unit tests to ensure that removing the dead docs still sees the correct revision being used.
| @@ -38,12 +38,12 @@ func (s *metricsDebugSuite) SetUpTest(c *gc.C) { | ||
| } | ||
| func (s *metricsDebugSuite) TestSetMeterStatus(c *gc.C) { | ||
| - testCharm := s.Factory.MakeCharm(c, &factory.CharmParams{Name: "metered", URL: "local:quantal/metered"}) | ||
| + testCharm := s.Factory.MakeCharm(c, &factory.CharmParams{Name: "metered", URL: "local:quantal/metered-1"}) |
wallyworld
Jan 25, 2017
Owner
There are other places that may need the same change, eg apiserver/uniter_test
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$test-infra-error$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
mjs
added some commits
Jan 24, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
mjs commentedJan 25, 2017
•
Edited 1 time
-
mjs
Jan 26, 2017
Instead of relying on "dead" charm documents, which doesn't work well with migrations, local charm revisions are now tracked using a sequence. Dead charm documents are now removed.
An upgrade step is also included which adds any required sequences for pre-existing charms.
This PR also includes a fix for an unrelated gofmt issue in worker/machiner.
QA steps
Bug reference
This is the final fix for https://bugs.launchpad.net/juju/+bug/1657614