Reindex dashboards and panels (allow migrations to queue a job to queue other indexing jobs)
ClosedPublic
Actions

Authored by epriestley on Apr 12 2019, 9:27 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13272: Remove the hard-coded homepage, and other Dashboards improvements

Commits

rP4eab3c4c8d2b: Reindex dashboards and panels (allow migrations to queue a job to queue other…

Summary

Depends on D20411. Ref T13272. Dashboards and panels have new indexes (Ferret and usage edges) that need a rebuild.

For large datasets like commits we have the "activity" flow in T11932, but realistically these rebuilds won't take more than a few minutes on any realistic install so we should be able to just queue them up as migrations.

Let migrations insert a job to basically run bin/search index --type SomeObjectType, then do that for dashboards and panels.

(I'll do Herald rules in a followup too, but I want to tweak one indexing thing there.)

Test Plan

Ran the migration, ran bin/phd debug task, saw everything get indexed with no manual intervention.

Diff Detail

Repository

rP Phabricator

Branch

panel14

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 22574
Build 30920: Run Core Tests
Build 30919: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	16 ms	PhabricatorWorkerTestCase::testLeasedIsOldestFirst Assertion failed, expected values to be equal (at PhabricatorWorkerTestCase.php:193): Tasks which expired earlier should lease first, all else being equal. Expected: 3 Actual: 6
	11 ms	PhabricatorWorkerTestCase::testMultipleLease Assertion failed, expected values to be equal (at PhabricatorWorkerTestCase.php:186): We should not be able to lease a task multiple times. Expected: 0 Actual: 1
	17 ms	PhabricatorWorkerTestCase::testNewBeforeLeased Assertion failed, expected values to be equal (at PhabricatorWorkerTestCase.php:193). Expected: 3 Actual: 5
	13 ms	PhabricatorWorkerTestCase::testTooManyTaskFailures Assertion failed, expected values to be equal (at PhabricatorWorkerTestCase.php:193). Expected: 3 Actual: 5
	13 ms	PhabricatorWorkerTestCase::testWaitBeforeRetry Assertion failed, expected values to be equal (at PhabricatorWorkerTestCase.php:186). Expected: 0 Actual: 1
		View Full Test Results (5 Failed · 359 Passed)

Event Timeline

epriestley created this revision.Apr 12 2019, 9:27 PM

Owners added a subscriber: Restricted Owners Package.Apr 12 2019, 9:27 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 12 2019, 9:29 PM

Harbormaster failed remote builds in B22574: Diff 48704!

Make the worker unit tests truncate the table before they run; otherwise, they get confused by the tasks inserted by these migrations.

Harbormaster completed remote builds in B22575: Diff 48705.Apr 12 2019, 11:02 PM

epriestley requested review of this revision.Apr 12 2019, 11:02 PM

epriestley added a child revision: D20413: Index and show Owners packages affected by Herald rules.Apr 12 2019, 11:17 PM

amckinley accepted this revision.Apr 17 2019, 5:37 PM

amckinley added inline comments.

resources/sql/autopatches/20190412.dashboard.13.rebuild.php
4–8	Maybe put these in a `try/catch` so users don't end up with a failed migration if this doesn't work for some reason?

This revision is now accepted and ready to land.Apr 17 2019, 5:37 PM

epriestley added inline comments.Apr 17 2019, 7:05 PM

resources/sql/autopatches/20190412.dashboard.13.rebuild.php
4–8	If there's some reason this may fail I'd like to learn about it and fix it, ideally. If someone reports something dumb that we can't really fix (or detect?) we could try/catch our way around it, but these shouldn't fail for any legitimate reason I'm aware of.

Closed by commit rP4eab3c4c8d2b: Reindex dashboards and panels (allow migrations to queue a job to queue other… (authored by epriestley). · Explain WhyApr 17 2019, 7:05 PM

This revision was automatically updated to reflect the committed changes.

(And the reindex is safe/idempotent, so some reasonable failures like "disk full" would be bad to try/catch our way through, since the correct remedy is to free some space and retry the migration.)

Or "db002, which has the worker database, is being super flaky, even though db001 with every other database is up" or something, although that may not actually lead to a possible state where the INSERT can fail and the patch can still be marked as successful.