Page MenuHomePhabricator

When queries overheat, raise an exception
ClosedPublic

Authored by epriestley on Mar 18 2019, 9:40 PM.
Tags
None
Referenced Files
F14103636: D20294.id48448.diff
Tue, Nov 26, 10:20 PM
F14103634: D20294.id48436.diff
Tue, Nov 26, 10:20 PM
F14103633: D20294.id.diff
Tue, Nov 26, 10:20 PM
F14103632: D20294.diff
Tue, Nov 26, 10:20 PM
Unknown Object (File)
Oct 27 2024, 7:56 PM
Unknown Object (File)
Oct 17 2024, 4:08 AM
Unknown Object (File)
Oct 15 2024, 3:11 PM
Unknown Object (File)
Sep 19 2024, 2:33 PM
Subscribers
None

Details

Summary

Ref T13259. Currently, queries set a flag and return a partial result set when they overheat. This is mostly okay:

  • It's very unusual for queries to overheat if they don't have a real viewer.
  • Overheating is rare in general.
  • In most cases where queries can overheat, the context is a SearchEngine UI, which handles this properly.

In T13259, we hit a case where a query with an omnipotent viewer can overheat: if you have more than 1,000 consecutive commits in the database with invalid repositoryID values, we'll overheat and bail out. This is pretty bad, since we don't process everything.

Change this beahvior:

  • Throw by default, so this stuff doesn't slip through the cracks.
  • Handle the SearchEngine case explicitly ("it's okay to overheat, we'll handle it").
  • Make QueryIterator disable overheating behavior: if we're iterating over all objects, we want to hit the whole table even if most of it is garbage.

There are some cases where this might cause new exception behavior that we don't necessarily want. For example, in Owners, each package shows "recent commits in this package". If you can't see the first 1,000 recent commits, you'd previously get a slow page with no results. Now you'll probably get an exception.

If these crop up, I think the best approach for now is to deal with them on a case-by-case basis and see how far we get. In the "Owners" case, it might be good to query by repositories you can see first, then query by commits in the package in those repositories. That should give us a better outcome than any generic behavior we could implement.

Test Plan
  • Added 100000 to all repositoryID values for commits on my local install.
  • Before making changes, ran bin/repository rebuild-identities --all --trace. Saw the script process 1,000 rows and exit silently.
  • Applied the first part ("throw by default") and ran bin/repository rebuild-identities. Saw the script process 1,000 rows, then raise an exception.
  • Applied the second part ("disable for queryiterator") and ran the script again. Saw the script process all 15,000 rows without issues (although none are valid and none actually load).
  • Viewed Diffusion, saw appropriate NUX / "overheated" UIs.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable