Generally LGTM, but see comments https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go File environs/ec2/ec2.go (right): https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode28 environs/ec2/ec2.go:28: longDelay: 5e9, These are ...
12 years, 2 months ago
(2012-02-24 16:51:56 UTC)
#2
12 years, 2 months ago
(2012-02-24 17:28:37 UTC)
#3
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode71
environs/ec2/ec2.go:71: err := longAttempt.do(
On 2012/02/24 16:51:56, fwereade wrote:
> Again I'm not 100% sure about this, but these anonymous functions are big
enough
> that I think it'd be nicer to define isTransient and updateDNSName outside the
> call.
i see where you're coming from, but i'm not sure.
the way i'm thinking of these attempt calls is that they're like loops.
if i'd written
for err == noDNS || err == environs.ErrMissingInstance {
insts, err := inst.e.Instance(...)
if err != nil {
break
}
etc
}
then would you be still be saying it would look better with the body inside
separate function?
if the code was in a separate function, it perhaps wouldn't be so clear how the
error logic worked with respect to the do semantics.
thinking.
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode219
environs/ec2/ec2.go:219: func() error {
On 2012/02/24 16:51:56, fwereade wrote:
> ...and the same again, with rather more intensity in this particular case :).
i think i agree here.
https://codereview.appspot.com/5694068/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):
https://codereview.appspot.com/5694068/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:59: // stuffed in fact.
On 2012/02/24 16:51:56, fwereade wrote:
> It feels like there's something problematic here... if we do manage to
bootstrap
> again, it seems to me that we definitely are stuffed (and while this is a juju
> problem, it's not one we're going to address here and now).
>
> In this instance, what's the benefit of possibly stuffing ourselves rather
than
> just waiting 5s and checking once? IMO a time savings of <5s is not really
worth
> it in this context...
this should be a call to ShortDo. i thought i'd done that actually.
hmm, forgot to publish these comments. also, as per discussion on IRC, changed attempt API. ...
12 years, 2 months ago
(2012-03-08 20:05:45 UTC)
#7
hmm, forgot to publish these comments.
also, as per discussion on IRC, changed attempt API.
PTAL
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode28
environs/ec2/ec2.go:28: longDelay: 5e9,
On 2012/02/24 16:51:56, fwereade wrote:
> These are enough times that I'd half-recommend defining "var seconds = 1e9"
and
> var minutes = 60*seconds" and defining other times in terms of those; but it'd
> also be reasonable to consider that just noise. Undecided.
Done.
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode177
environs/ec2/ec2.go:177: err = shortAttempt.do(hasCode("InvalidGroup.NotFound"),
func() error {
On 2012/02/24 16:51:56, fwereade wrote:
> Again I'd find it more readable if you defined a runInstances function outside
> the actual call.
i don't agree here - it's just one statement.
https://codereview.appspot.com/5694068/diff/1/environs/ec2/ec2.go#newcode219
environs/ec2/ec2.go:219: func() error {
On 2012/02/24 17:28:37, rog wrote:
> On 2012/02/24 16:51:56, fwereade wrote:
> > ...and the same again, with rather more intensity in this particular case
:).
>
> i think i agree here.
Done.
https://codereview.appspot.com/5694068/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):
https://codereview.appspot.com/5694068/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:59: // stuffed in fact.
On 2012/02/24 17:28:37, rog wrote:
> On 2012/02/24 16:51:56, fwereade wrote:
> > It feels like there's something problematic here... if we do manage to
> bootstrap
> > again, it seems to me that we definitely are stuffed (and while this is a
juju
> > problem, it's not one we're going to address here and now).
> >
> > In this instance, what's the benefit of possibly stuffing ourselves rather
> than
> > just waiting 5s and checking once? IMO a time savings of <5s is not really
> worth
> > it in this context...
>
> this should be a call to ShortDo. i thought i'd done that actually.
ah, of course it can't be a call to ShortDo as it's in jujutest. i'm slightly
reluctant to just wait for 5s as this test is run in local mode too and that
will instantly quadruple the length of time that test takes to run (this test is
run three times).
ok, i've added a ConsistencyDelay field to the LiveTests struct, and slept for
that long.
Looking pretty good. Here is a round of ideas: https://codereview.appspot.com/5694068/diff/7001/environs/config_test.go File environs/config_test.go (right): https://codereview.appspot.com/5694068/diff/7001/environs/config_test.go#newcode92 environs/config_test.go:92: ...
12 years, 2 months ago
(2012-03-09 09:29:40 UTC)
#8
12 years, 2 months ago
(2012-03-09 14:33:51 UTC)
#9
PTAL
https://codereview.appspot.com/5694068/diff/7001/environs/config_test.go
File environs/config_test.go (right):
https://codereview.appspot.com/5694068/diff/7001/environs/config_test.go#newc...
environs/config_test.go:92: addr, _ := i0.DNSName()
On 2012/03/09 09:29:41, niemeyer wrote:
> Please check the error too. c.Assert(err, IsNil) is cheap.
we do know that dummyInstance.DNSName can never return an error, but done
anyway.
https://codereview.appspot.com/5694068/diff/7001/environs/config_test.go#newc...
environs/config_test.go:103: addr, _= i1.DNSName()
On 2012/03/09 09:29:41, niemeyer wrote:
> Ditto.
Done.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode26
environs/ec2/ec2.go:26: delay: 5 * time.Second,
On 2012/03/09 09:29:41, niemeyer wrote:
> We can use 1 second here. It's extremely lightweight already.
Done.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode63
environs/ec2/ec2.go:63: func (inst *instance) DNSName() (string, error) {
On 2012/03/09 09:29:41, niemeyer wrote:
> Having such a call returning 3 minutes by default at all times might be an
> issue. We have logic in the "status" command, for example, that is supposed to
> tell the user that the machine is still pending an address.
>
> The implementation looks good now, though.
added WaitDNSName as discussed.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode72
environs/ec2/ec2.go:72: if err == environs.ErrMissingInstance {
On 2012/03/09 09:29:41, niemeyer wrote:
> Same thing here. There's an application running somewhere, we shutdown a
> machine, and then that app hangs for 3 minutes just because it asked for a DNS
> address? Waiting that long by default on such a call feels like a trap waiting
> to catch us. I think we should try to address the needs of eventual
consistency
> here, but not of a starting machine that will take a long time.
>
> Let's talk online about that.
Done.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode147
environs/ec2/ec2.go:147: addr, err := inst.DNSName()
On 2012/03/09 09:29:41, niemeyer wrote:
> This is a place that can wait longer, but here it's waiting for *all* of them
to
> become available, which in a way spoils the very reason why we'd want to have
> more than one of these up. It should wait, but stop on the first batch that
has
> at least one valid DNSName.
Done.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode183
environs/ec2/ec2.go:183: if err == nil || ec2ErrCode(err) !=
"InvalidGroup.NotFound" {
On 2012/03/09 09:29:41, niemeyer wrote:
> Very nice.
i'm happy with the way the attempt stuff turned out. thanks for the push!
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode256
environs/ec2/ec2.go:256: // each request should add more instances to the
requested
On 2012/03/09 09:29:41, niemeyer wrote:
> s/should/attempts to/?
Done.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/ec2.go#newcode266
environs/ec2/ec2.go:266: // If any slot has been filled, we return the insts
slice.
On 2012/03/09 09:29:41, niemeyer wrote:
> This logic is error prone. When we get ErrMissingInstance, we'll have to
> *always* check len(insts) to see if it's 0 or not before iterating, or we risk
a
> panic that may be hidden before actual deployment in production.
>
> To avoid that problem, it should consistently return the slice of insts, even
if
> all of the entries are nil, or it should have different error values for some
> instances vs. no instances.
added another error value, as discussed on IRC.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:60: var ShortAttempt =
AttemptStrategy{&shortAttempt}
On 2012/03/09 09:29:41, niemeyer wrote:
> It doesn't look like exporting these is needed.
ShortAttempt is used, so i thought i may as well export both. changed to export
only ShortAttempt.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:217: insts, err = t.Env.Instances([]string{inst0.Id(),
inst2.Id()})
On 2012/03/09 09:29:41, niemeyer wrote:
> Isn't Instances retrying internally? Why do we have a reimplementation of its
> internal logic here?
because we're waiting for Instances to return an error. If Instances succeeds,
it doesn't retry. i've added a comment.
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/util.go
File environs/ec2/util.go (right):
https://codereview.appspot.com/5694068/diff/7001/environs/ec2/util.go#newcode46
environs/ec2/util.go:46: // attempt represents a strategy for waiting for an ec2
request to complete
On 2012/03/09 09:29:41, niemeyer wrote:
> s/attempt/&Strategy/
done
> This description is also describing a problem that happens in EC2, which is
good
> to have documented, but that's not the documentation for attemptStrategy.
i've moved that part of the comment to shortAttempt.
Issue 5694068: environs/ec2: make tests work against amazon server
Created 12 years, 2 months ago by rog
Modified 12 years, 2 months ago
Reviewers: mp+94566_code.launchpad.net
Base URL:
Comments: 38
