Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1201)

Issue 11923043: Standardize storage-consistency polling strategy. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by jtv.canonical
Modified:
10 years, 9 months ago
Reviewers:
mp+177163, thumper, rog
Visibility:
Public.

Description

Standardize storage-consistency polling strategy. All providers have a "shortAttempt" polling strategy, which they use both to wait for eventual storage consistency and for generally retrying things that may take a while to become possible. These package-global variables were getting in the way of the factoring-out of common code. And so, as discussed in https://codereview.appspot.com/11633043/ this branch makes the how-to-wait-for-storage-consistency part available on the StorageReader interface. VerifyBootstrapInit no longer needs to take the strategy as an argument, because it's available on the environment's storage object. Some shortAttempt variables are replaced entirely; in other cases we now have to different polling strategies that probably deserve to be tuned for their specific purposes. I also found a new way to specify non-delaying policies. There was always the problem that even in testing, we needed a little bit of delay because otherwise an attempt strategy might decide immediately that it was done, and loops might not be executed at all. The "little bit of delay" made behaviour nondeterministic, leading to the risk of spurious failures on slow or busy systems if it was too small, and slowing down the test suite if it was too large. But there's also a Min field, which sets a minimum number of times the loop must be attempted. For a non-delaying attempt strategy, the right approach seems to be to specify zero delays but a Min of 1. Where providers need storage polling that actually sleeps, I made sure they override these with non-delaying strategies. For the providers with immediately-consistent storage there is no delay at all, and so no need to override the strategy either. This branch will conflict with the one whose review prompted this work. Unfortunate, but better than a big conflated giga-branch. https://code.launchpad.net/~jtv/juju-core/storage-consistency-strategy/+merge/177163 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Standardize storage-consistency polling strategy. #

Total comments: 7

Patch Set 3 : Standardize storage-consistency polling strategy. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -67 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M environs/azure/azure_test.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/azure/environ.go View 4 chunks +2 lines, -14 lines 0 comments Download
M environs/azure/storage.go View 1 2 chunks +10 lines, -0 lines 0 comments Download
M environs/bootstrap.go View 1 2 2 chunks +6 lines, -4 lines 0 comments Download
M environs/dummy/environs.go View 1 chunk +2 lines, -5 lines 0 comments Download
M environs/dummy/storage.go View 1 2 chunks +9 lines, -2 lines 0 comments Download
M environs/ec2/ec2.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/export_test.go View 1 chunk +4 lines, -1 line 0 comments Download
M environs/ec2/local_test.go View 3 chunks +3 lines, -3 lines 0 comments Download
M environs/ec2/storage.go View 1 2 4 chunks +17 lines, -1 line 0 comments Download
M environs/emptystorage.go View 1 2 chunks +6 lines, -0 lines 0 comments Download
M environs/interface.go View 2 chunks +8 lines, -0 lines 0 comments Download
M environs/local/environ.go View 2 chunks +2 lines, -12 lines 0 comments Download
M environs/localstorage/storage.go View 1 2 chunks +9 lines, -2 lines 0 comments Download
M environs/maas/environ.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M environs/maas/storage.go View 1 2 chunks +13 lines, -3 lines 0 comments Download
M environs/openstack/export_test.go View 1 chunk +4 lines, -1 line 0 comments Download
M environs/openstack/local_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M environs/openstack/provider.go View 1 2 2 chunks +5 lines, -2 lines 0 comments Download
M environs/openstack/storage.go View 1 3 chunks +17 lines, -3 lines 0 comments Download
M environs/sync/sync.go View 1 2 chunks +6 lines, -0 lines 0 comments Download
M environs/testing/polling.go View 1 2 1 chunk +2 lines, -9 lines 0 comments Download
M utils/attempt.go View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 6
jtv.canonical
Please take a look.
10 years, 9 months ago (2013-07-26 15:33:29 UTC) #1
jtv.canonical
Please take a look.
10 years, 9 months ago (2013-07-26 16:20:43 UTC) #2
rog
LGTM with some suggestions below. I think this is working out quite nicely, thanks. https://codereview.appspot.com/11923043/diff/4001/environs/bootstrap.go ...
10 years, 9 months ago (2013-07-26 16:34:12 UTC) #3
jtv.canonical
Please take a look.
10 years, 9 months ago (2013-07-26 16:46:53 UTC) #4
jtv.canonical
On 2013/07/26 16:34:12, rog wrote: > https://codereview.appspot.com/11923043/diff/4001/environs/bootstrap.go#newcode53 > environs/bootstrap.go:53: for a := env.Storage().ConsistencyStrategy().Start(); > a.Next(); ...
10 years, 9 months ago (2013-07-26 16:51:25 UTC) #5
thumper
10 years, 9 months ago (2013-07-29 03:01:05 UTC) #6
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b