On Friday 20 August 2010 11:43:40 you wrote:
> On Thu, Aug 19, 2010 at 5:13 PM, Julian Edwards
> 
> <email address hidden> wrote:
> > On Thursday 19 August 2010 13:37:40 you wrote:
> >> Review: Needs Fixing
> >> Hey Julian,
> >> 
> >> This looks like a big step toward build farm reliability -- thank you.
> 
> Thanks for the changes. There are a few questions below, but I reckon
> this'll be the last round.

I'm not so sure :)

> 
> >> Some of my comments are asking for further explanation, a few make
> >> suggestions for moving some code around, and the rest are the normal
> >> stylistic gotchas.
> >> 
> >> Out of curiosity, did you discuss this change in detail with anyone
> >> before landing?
> > 
> > It's been so long since I started it I honestly can't remember.  I have
> > vague recollections of mentioning failure counts to various people.
> 
> No worries. I ask because we have failure counting mechanisms in other
> places in Launchpad (e.g. the branch puller) and it might have been
> interesting to share patterns, variable names and the like.

I had no idea about that, that's a shame :(

> >> > === modified file 'lib/lp/buildmaster/manager.py'
> >> > --- lib/lp/buildmaster/manager.py   2010-08-02 16:00:50 +0000
> >> > +++ lib/lp/buildmaster/manager.py   2010-08-18 17:01:23 +0000
> 
> ...
> 
> >> > +        :return: True if we disabled something, False if we did not.
> >> 
> >> This seems like an odd thing to return. It doesn't seem to be used in
> >> the code. What's it for?
> > 
> > For the tests.
> 
> Yeah, but what about the tests? Or rather, why do the tests care about
> something that the code doesn't?

Well lots of our tests return stuff that the code doesn't need, like Deferreds 
for example.  But fine, I've got rid of it.

> >> > +            # Decide if we need to terminate the job or fail the
> >> > +            # builder.
> >> > +            self._incrementFailureCounts(builder)
> >> > +            self.logger.info(
> >> > +                "builder failure count: %s, job failure count: %s" %
> >> > ( +                    builder.failure_count,
> >> > +
> >> > builder.currentjob.specific_job.build.failure_count)) +
> >> > BaseDispatchResult(slave=None).assessFailureCounts(builder)
> >> 
> >> This line makes me wonder why assessFailureCounts is on
> >> BaseDispatchResult at all.  Perhaps it should be a method on Builder
> >> that takes an optional 'info' parameter?
> >> 
> >> The above code would then read:
> >> 
> >>   builder.gotFailure()
> >>   self.logger.info(...)
> >>   builder.assessFailureCounts()
> > 
> > I don't think we should continue to bloat model classes with code that is
> > only used in one place.  This is a buildd-manager-specific function.
> > 
> > Anyway, bugger, I had intended to factor it out to a standalone function
> > as I also realised that that line is ridiculous and again forgot :/
> 
> A standalone function would be a definite improvement, and fine by me.
> I personally don't think it's bloat to say "a builder knows how to
> handle builder failures", but I'm not going to push it.

By the same argument, anything that deals with object X should be a method on 
that object X.

The difference here is that it's not dealing with builder failures 
exclusively, it's dealing with a combination of builder and job failures, and 
I would personally find it odd that a builder was trying to modify job 
properties.

Generally, I think the boundaries of responsibility in our code are poor and 
should be less like this.

Did I convince you yet?  Probably not :)

> >> Also, perhaps there should be a current_build property on Builder.
> >> 
> >>   @property
> >>   def current_build(self):
> >>       return self.currentjob.specific_job.build
> > 
> > I don't really like that either I'm afraid!  It's masking three
> > complicated queries here and dangerous to have as a property as it lulls
> > you into a false sense of it really being a cheap operation.
> 
> I didn't know they were queries!
> 
> > I don't mind making it a regular method though, getCurrentBuildFarmJob()
> 
> That would be great. Out of curiosity, why such a long name? Do
> builders ever have jobs that aren't on build farms?

BuildFarmJob is the model object name.  There are other *Job tables so this 
makes it specific which one you're getting.

> 
> >> > +        builder.failure_count += 1
> >> > +        builder.currentjob.specific_job.build.failure_count += 1
> >> > +
> >> 
> >> Perhaps this should be a method on Builder. e.g.
> >> 
> >>   class Builder:
> >> 
> >>      # ...
> >> 
> >>      def gotFailure(self):
> >>          self.failure_count += 1
> >>          self.currentjob.specific_job.build.failure_count += 1
> >> 
> >> Likewise, there could also be a gotSuccess() that resets to 0.
> > 
> > For the same reasons as before, I don't think this belongs on IBuilder.
> >  Not to mention it's not just manipulating builder properties, it's
> > changing the job's properties.
> 
> In answer to the second point:
>   def gotFailure(self):
>     self.failure_count += 1
>     self.getCurrentBuild().gotFailure()

I still don't like it :( It's mixing responsibilities with what needs to 
happen on failures, which is something that only the build manager should be 
thinking about.

I'd prefer to have a gotFailure() on each of Builder and BuildFarmJob and have 
the build manager call each in turn.

Let me know if you think that is acceptable and I'll do that change (it's not 
in the attached diff).

> 
> And as for bloat, I don't see why adding extra columns is not bloat.

Sorry, I don't understand what you mean by that.

> >> > === modified file 'lib/lp/buildmaster/tests/test_manager.py'
> >> > --- lib/lp/buildmaster/tests/test_manager.py        2010-08-06
> >> > 10:48:49 +0000 +++ lib/lp/buildmaster/tests/test_manager.py      
> >> >  2010-08-18 17:01:23 +0000
> >> 
> >> ...
> >> 
> >> > @@ -360,9 +360,17 @@
> >> > 
> >> >              self.assertFalse(result.processed)
> >> > 
> >> >          return d.addCallback(check_result)
> >> > 
> >> > -    def testCheckDispatch(self):
> >> > -        """`SlaveScanner.checkDispatch` is chained after dispatch
> >> > requests. -
> >> > +    def _setUpSlaveAndBuilder(self):
> >> > +        # Helper function to set up a builder and its recording
> >> > slave. +        slave = RecordingSlave('bob',
> >> > 'http://foo.buildd:8221/', 'foo.host') +        bob_builder =
> >> > getUtility(IBuilderSet)[slave.name] +        return slave,
> >> > bob_builder
> >> > +
> >> 
> >> There's already a slave called 'bob' in the sampledata?
> > 
> > There's a builder called 'bob'.  There are no slaves.
> > 
> > There's also another called 'frog'.
> 
> OK. As long as there are meaningfully named constants, I'm happy. When
> reading the test in the first place, one can't tell whether that
> hostname and port number is significant or not.

OK, I'll make them obvious via intent-revealing variable names.

> 
> >> Also, it might be a good idea to have this method guarantee that the
> >> failure counts are zero.
> > 
> > I considered that, but the methods that call this one set the counts to
> > varying values.
> 
> I see that, but I do think it would be clearer, and less surprising
> for anyone who adds more tests in future.

I've re-jigged it so that the counts are passed to the setup method.

> >> > +        manager.scheduleNextScanCycle = FakeMethod()
> >> > +        self.patch(BaseDispatchResult, 'assessFailureCounts',
> >> > FakeMethod())
> >> 
> >> Why are you stubbing out assessFailureCounts here?
> > 
> > Because at the bottom of the method you'll see that it's checking the
> > call_count.
> 
> Yeah, but why are you checking that it's called? Just as easy and more
> robust to check that the state is what you expect.

I think it's less robust like that.  The expected state after calling that 
function is checked elsewhere.  Now, these tests can just check that it's 
called at all and be confident that it will DTRT.  This means that if we tweak 
the method later, it will only require one test to be fixed instead of fixing 
all the tests that duplicate the same check.  (this is something that 
particularly pisses me off with Soyuz tests and I am slowly fixing them all)

If you think this is wrong, I am open as to why you think so.

> >> >          builder, job_id = self._getBuilder('bob')
> >> > 
> >> >          # Setup a interaction to satisfy 'write_transaction'
> >> > decorator. login(ANONYMOUS)
> >> >          slave = RecordingSlave(builder.name, builder.url,
> >> >          builder.vm_host) result = FailDispatchResult(slave, 'does not
> >> >          work!')
> >> > 
> >> > +        result.assessFailureCounts = FakeMethod()
> >> 
> >> Why are you stubbing it out here?
> > 
> > So that I can see if it got called.
> 
> As above, why do you care?

See above.

> 
> >> > +        self.assertEqual(0, result.assessFailureCounts.call_count)
> >> 
> >> This assertion seems a little redundant.
> > 
> > Well, it's fairly standard testing practice, no?  How else do I know that
> > calling result() has changed the count?
> 
> By checking to see that the count is different.

See above.

> 
> > === modified file 'lib/lp/buildmaster/manager.py'
> > --- lib/lp/buildmaster/manager.py	2010-08-18 16:58:18 +0000
> > +++ lib/lp/buildmaster/manager.py	2010-08-19 15:51:41 +0000
> > @@ -141,10 +141,55 @@
> > 
> >          return d
> > 
> > +def get_builder(name):
> > +    """Helper to return the builder given the slave for this request."""
> > +    # Avoiding circular imports.
> > +    from lp.buildmaster.interfaces.builder import IBuilderSet
> > +    return getUtility(IBuilderSet)[name]
> > +
> > +
> > +def assessFailureCounts(builder, fail_notes):
> > +    """View builder/job failure_count and work out which needs to die.
> > +
> > +    :return: True if we disabled something, False if we did not.
> > +    """
> > +    current_job = builder.currentjob
> > +    build_job = current_job.specific_job.build
> > +
> 
> Or indeed, build_job = builder.getCurrentBuildFarmJob()

That would incur duplicated queries! (via getting currentjob twice)  It's why 
I've split it up as I did.

I think performance should trump things here but perhaps you can think of a 
better way?

> > === modified file 'lib/lp/buildmaster/tests/test_manager.py'
> > --- lib/lp/buildmaster/tests/test_manager.py	2010-08-18 16:58:18 +0000
> > +++ lib/lp/buildmaster/tests/test_manager.py	2010-08-19 16:06:10 +0000
> > @@ -7,7 +7,6 @@
> > 
> >  import signal
> >  import time
> >  import transaction
> > 
> > -import unittest
> > 
> >  from twisted.internet import defer, reactor, task
> >  from twisted.internet.error import ConnectionClosed
> > 
> > @@ -38,6 +37,7 @@
> > 
> >  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> >  from lp.testing.factory import LaunchpadObjectFactory
> >  from lp.testing.fakemethod import FakeMethod
> > 
> > +from lp.testing.sampledata import BOB_THE_BUILDER_NAME
> 
> There's got to be a better name. What are the interesting properties of the
> builder that make it worth using?

None that I know of, there's just two boring old builders in the sample data.

> 
> > @@ -362,7 +363,8 @@
> > 
> >      def _setUpSlaveAndBuilder(self):
> >          # Helper function to set up a builder and its recording slave.
> > 
> > -        slave = RecordingSlave('bob', 'http://foo.buildd:8221/',
> > 'foo.host') +        slave = RecordingSlave(
> > +            BOB_THE_BUILDER_NAME, 'http://foo.buildd:8221/', 'foo.host')
> > 
> >          bob_builder = getUtility(IBuilderSet)[slave.name]
> >          return slave, bob_builder
> 
> As mentioned in my reply, are these URLs or domain names special in any
> way? If not, why the port number?

I've given them intent-revealing names as I mentioned above.

So, partial diff attached again!  Let me know what you think about my 
questions in reply to your questions :)

Cheers
j