launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

73	spph = ClassAlias(SourcePackagePublishingHistory, "spph")

I don't understand why this needs to be a ClassAlias; if you just want a shorter Python name for the class without aliasing in SQL then 'spph = SourcePackagePublishingHistory' is fine and relatively idiomatic.

113	+ SourcePackageRelease.dateuploaded, Alias(spph.id, 'spph_id')),

The custom alias seems pointless here.

114	+ spph.id > self.next_spph_id

That sounds, then, rather like it's actually last_spph_id.

132	+ return self.getPendingUpdates().count() == 0

We don't care about the exact count. is_empty() is cheaper and does what we want.

134	- def update_cache(self, updates):
135	+ def update_cache(self, update, inserts):

This should be updateCache.

167	+ def perform_update(spr_id, creator_id, maintainer_id, archive_id,
168	+ purpose, distroseries_id, spn_id, dateuploaded,
169	+ spph_id):

The update here is perhaps slightly excessive. The only fields that can ever change are publication, date_uploaded, and sourcepackagerelease, and it will overwrite all but date_uploaded even if the old record is newer. I suspect we want the update to be conditional on its dateuploaded being newer (for correctness), and to avoid setting the immutable columns (for compactness and efficiency).

Additionally, you probably want to aggregate the inserts; I think the current code will crash if two of the same key show up new in a single batch. There's also likely to be benefit in applying the same aggregation technique to updates, not to avoid crashes but to avoid duplicating work. update_cache will likely end up compact enough to be inlined.

It's probably also cleaner to reword the update as a self.store.find(LPSPRC, LPSPRC.upload_archive_id == archive_id, [...], LPSPRC.dateuploaded < dateuploaded).set(dateuploaded=dateuploaded, [...]).

I'd lastly like to see the Storm class renamed to LatestPersonSourcePackageReleaseCache (note the 'Package' rather than 'package'), as the existing compound concept capitalisation scheme was phased out and exterminated from the codebase in around 2005.

209	+ for update in (self.getPendingUpdates()[:chunk_size]):

Extraneous parens are extraneous.

245	- self.runDaily()
246	- self.runHourly()
247	+# self.runDaily()
248	+# self.runHourly()

This seems like an accidental change.
-- 
https://code.launchpad.net/~wallyworld/launchpad/another-reporting-cache-garbojob-1071581/+merge/133859
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.