Code review comment for lp://staging/~rvb/launchpad/branches-timeout-bug-827935-3

Revision history for this message
Abel Deuring (adeuring) wrote :

(14:10:17) adeuring: rvba: the changes look good, but a suggestion nevertheless: What about making is_branch_count_zero a @cachedproperty? after all, its accessed more than once and it calls a method. (I hva no idea though how expensive the method is...)
(14:12:01) rvba: adeuring: well, the method is definitely no expensive: because self.branches().visible_branches_for_view and self.branch_count are @cacheproperty.
(14:12:08) rvba: s/no/not/
(14:12:34) adeuring: rvba: ah, ok, so let's leave the @property
(14:12:48) adeuring: rvba: another question: If branch_count needs to be evaluated in is_branchcount__zero: DO you expect the query in these cases to be faster?
(14:13:23) adeuring: ...if not, would it make sense to call resulstset.any() instead?
(14:14:51) rvba: adeuring: I'm not sure I understand your suggestion, this branch does not bring any improvement to the cases where branch_count needs to be evaluated.
(14:15:54) rvba: adeuring: When the batch is empty that is. But hopefully this wont happen in many cases because when the number of branches is huge, chances are that you have all the statuses represented and the only filtering possible is by status.
(14:17:17) adeuring: rvba: right; i I just wondered if it would be hard to add that case too. But that's probably my bad habit to squeeze too much changes into one branch ;)
(14:17:22) adeuring: rbva: r=me then

review: Approve

« Back to merge proposal