launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06706
[Merge] lp:~wgrant/launchpad/bug-919116 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-919116 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #919116 in Launchpad itself: "BuilderSet:+index KeyError in preloadSpecificJobData"
https://bugs.launchpad.net/launchpad/+bug/919116
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-919116/+merge/97587
Bug #919116 has eluded diagnosis for some time, but I finally worked it out today. The BuilderSet:+index preloading code was assumes that Job.id == BuildQueue.id == SourcePackageRecipeBuild.id, which the lack of sampledata in those tables conveniently made true in the test suite. Most of the time this short-circuits because none are found, but occasionally a random row is found and the rest of the preloading blows up. So the preloading either crashes or doesn't do anything at all :)
I fixed the invalid assumptions and adjusted the tests to confirm that the query counts are constant. specific_jobs was also being evaluated a few times, so I changed it to listify once.
--
https://code.launchpad.net/~wgrant/launchpad/bug-919116/+merge/97587
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-919116 into lp:launchpad.
=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py 2012-01-04 16:39:15 +0000
+++ lib/lp/buildmaster/model/buildqueue.py 2012-03-15 08:57:26 +0000
@@ -45,6 +45,7 @@
IBuildQueue,
IBuildQueueSet,
)
+from lp.services.database.bulk import load_related
from lp.services.database.constants import DEFAULT
from lp.services.database.enumcol import EnumCol
from lp.services.database.sqlbase import (
@@ -157,12 +158,13 @@
for job_type, grouped_queues in groupby(queues, key=key):
specific_class = specific_job_classes()[job_type]
queue_subset = list(grouped_queues)
+ job_subset = load_related(Job, queue_subset, ['jobID'])
# We need to preload the build farm jobs early to avoid
# the call to _set_build_farm_job to look up BuildFarmBuildJobs
# one by one.
- specific_class.preloadBuildFarmJobs(queue_subset)
- specific_jobs = specific_class.getByJobs(queue_subset)
- if len(list(specific_jobs)) == 0:
+ specific_class.preloadBuildFarmJobs(job_subset)
+ specific_jobs = list(specific_class.getByJobs(job_subset))
+ if len(specific_jobs) == 0:
continue
specific_class.preloadJobsData(specific_jobs)
specific_jobs_dict = dict(
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py 2012-01-04 20:12:28 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py 2012-03-15 08:57:26 +0000
@@ -444,7 +444,7 @@
)
return list(IStore(SourcePackageRecipeBuildJob).find(
SourcePackageRecipeBuild,
- [SourcePackageRecipeBuildJob.id.is_in([job.id for job in jobs]),
+ [SourcePackageRecipeBuildJob.job_id.is_in([j.id for j in jobs]),
SourcePackageRecipeBuildJob.build_id ==
SourcePackageRecipeBuild.id]))
=== modified file 'lib/lp/soyuz/browser/tests/test_builder.py'
--- lib/lp/soyuz/browser/tests/test_builder.py 2012-01-05 14:34:21 +0000
+++ lib/lp/soyuz/browser/tests/test_builder.py 2012-03-15 08:57:26 +0000
@@ -5,12 +5,13 @@
__metaclass__ = type
-from testtools.matchers import LessThan
+from testtools.matchers import Equals
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from lp.buildmaster.interfaces.builder import IBuilderSet
from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.services.job.model.job import Job
from lp.services.webapp import canonical_url
from lp.soyuz.browser.tests.test_builder_views import BuildCreationMixin
from lp.testing import (
@@ -67,10 +68,11 @@
layer = LaunchpadFunctionalLayer
- # XXX rvb: the query issued per build is the result of the call to
- # build.buildqueue_record. It was decided not to make it a cachedproperty
- # because the code relies on the fact that this property always returns
- # the current record.
+ def setUp(self):
+ super(TestBuildersHomepage, self).setUp()
+ # Create a non-buildfarm job to ensure that the BuildQueue and
+ # Job IDs differ, detecting bug #919116.
+ Job()
def test_builders_binary_package_build_query_count(self):
def create_build():
@@ -81,10 +83,7 @@
nb_objects = 2
recorder1, recorder2 = record_two_runs(
builders_homepage_render, create_build, nb_objects)
-
- self.assertThat(
- recorder2,
- HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
def test_builders_recipe_build_query_count(self):
def create_build():
@@ -95,10 +94,7 @@
nb_objects = 2
recorder1, recorder2 = record_two_runs(
builders_homepage_render, create_build, nb_objects)
-
- self.assertThat(
- recorder2,
- HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
def test_builders_translation_template_build_query_count(self):
def create_build():
@@ -106,16 +102,10 @@
branch = self.factory.makeBranch()
specific_job = jobset.create(branch)
queueset = getUtility(IBuildQueueSet)
- # Using rSP is required to get the job id.
- naked_job = removeSecurityProxy(specific_job.job)
- job_id = naked_job.id
- queue = queueset.get(job_id)
+ queue = queueset.getByJob(specific_job.job)
queue.markAsBuilding(self.factory.makeBuilder())
nb_objects = 2
recorder1, recorder2 = record_two_runs(
builders_homepage_render, create_build, nb_objects)
-
- self.assertThat(
- recorder2,
- HasQueryCount(LessThan(recorder1.count + 1 * nb_objects + 1)))
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))