launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

On Monday 02 August 2010 14:15:54 you wrote:
> >> The comment makes me think there should be a method called
> >> IBuilder.getActiveBuild().
> > 
> > I've made IBuildQueueSet.getByActiveBuilder()
> 
> Thanks. Why that and not IBuilder.getActiveBuild()?

Because it's returning BuildQueue objects, not builders.

> > As we agreed on the call, it's renamed to slaveDialogueEnded.
> 
> Thanks, although in code we use American spelling, which would make it
> slaveDialogEnded.

To avoid frustration on both sides of the Atlantic it's now
slaveConversationEnded().

> >> > @@ -423,76 +481,98 @@
> >> >         self.logger.error(
> >> >             '%s failed to dispatch (%s)' % (slave, info))
> >> > 
> >> > -        self.slaveDone(slave)
> >> > +        self.waitOnDeferredList()
> >> 
> >> I wonder whether each of these calls is actually needed. If they are
> >> all needed, how can you tell?
> > 
> > I can't remember what you're referring to here.
> 
> Basically, there's nothing at all to hint that this is an appropriate
> place to call slaveDialogEnded, and looking over the code, it seems
> that it would be very easy to accidentally miss one. Perhaps in a
> future branch, they'll all go through one choke-point.
> 
> In essence, this is a variant of "have a function that does a scan and
> returns a Deferred that fires when it's done".

Right, that makes a lot more sense, thanks.

> In the diff, you've made NewBuildersScanner inherit from TimeoutMixin.
> AFAICT, it isn't necessary, and only confuses things since there's no
> actual timeout involved.
> 
> Instead of mixing in, you could do the following in the constructor:
> 
>     def __init__(self, manager, clock=None):
>         self.manager = manager
>         if clock is None:
>             clock = reactor
>         self._clock = clock
> 
> And then change scheduleScan to just do:
>         return self._clock.callLater(self.SCAN_INTERVAL, self.scan)

Gar, thanks.  I got too distracted while looking at the RunProcessWithTimeout 
example!

My only concern with all of this, regardless of how it's done, is that there's 
always an untested line of code with this way of testing.  I can't think of a 
better way of doing it though.

> >> Why did you need to make this change?
> > 
> > As explained on the call - the old defer.fail was getting exectuted
> > immediately.  I've no idea why and wasted 2 days at the Epic trying to
> > figure it out :/
> 
> That sucks. I guess it's ok to leave it as it stands for now, but this
> code really needs to change.

Yes, it makes me cry.

When I get more enthusiasm for this stuff I'll attack these tests with a bit 
more vigour.

> 
> ...
> 
> >> What you should be doing instead is always, always, always returning
> >> Deferreds from things that make Deferreds or call something that
> >> returns a Deferred.
> > 
> > There's no need to tell *me* that now :)
> 
> Heh heh.

BTW, how does that pattern usually apply with a callLater() invocation?  Or 
doesn't it, and we pass the clock?

> > Same for me and mwh.  It needs a serious code re-org to start returning
> > Deferreds in the right places, and a total test re-write.
> > 
> > But now is not the right time :(
> 
> Yeah. Soon though.

See previous comment about enthusiasm.

> Thanks. I've referred to it while replying.
> 
> In summary:
>   * I don't understand why getByActiveBuilder is on IBuildQueueSet
> rather than being IBuilder.getActiveBuild().
>   * slaveDialogueEnded => slaveDialogEnded
>   * Don't inherit from TimeoutMixin in NewBuildersScanner.
> 
> Almost there!
> 
> jml

http://pastebin.ubuntu.com/472219/

How are we looking?

Cheers.

-- 
https://code.launchpad.net/~julian-edwards/launchpad/buildd-manager-parallel-scan/+merge/30672
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/buildd-manager-parallel-scan into lp:launchpad/devel.