Closed Bug 1318474 Opened 8 years ago Closed 8 years ago

Store the job metadata in the central database instead of in datasource

Categories

(Tree Management :: Treeherder, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(10 files, 1 obsolete file)

(deleted), text/x-github-pull-request
camd
: review+
Details
(deleted), text/x-github-pull-request
Details
(deleted), text/x-github-pull-request
Details
(deleted), text/x-github-pull-request
Details
(deleted), text/x-github-pull-request
camd
: review+
Details
(deleted), text/x-github-pull-request
Details
(deleted), text/plain
Details
(deleted), text/x-github-pull-request
camd
: review+
Details
(deleted), text/x-github-pull-request
camd
: review+
Details
(deleted), text/plain
Details
This is the last bit of information in the per-project databases, once this is done we should be able to fairly rip out the rest of datasource!
Based on my experiences with the result set / push migration, I'm going to go for a phased approach here: 1. Start ingesting job metadata in ORM alongside datasource. 2. Make endpoints *use* the data in the ORM, but still use the id numbering from the per-project datasource tables. 3. Make endpoints use/return the id numbering from the ORM. 4. Stop ingesting data into the per-project datasource tables. By doing this phased approach, I think we can get some of the benefits of the migration finished sooner (e.g. making the treeherder database useable/accessible from redash/activedata, facilitate new projects like SETA) and reduce the risk of everything going wrong by switching things over all at once. I may actually postpone (3) and (4) until December or later, but hoping to get (1) and (2) landed by next week.
Comment on attachment 8811959 [details] [treeherder] wlach:1318474 > mozilla:master Hey Cam, could you have a look at this? I think it'll be easier to review this work in chunks, instead of me generating a huge big bang patch which does everything at once. This PR adds all the job data we care about to the job table inside the main database, and starts using it for all API endpoints. When/if this PR lands, we can pretty much start ignoring datasource for all practical purposes (we basically only use it to number the jobs that we store in the main database).
Attachment #8811959 - Flags: review?(cdawson)
Sorry I've taken so long to get started on this. I've been trying to finish up the Persona migration work for Pulse Guardian before end of month. Starting to review this now.
Comment on attachment 8811959 [details] [treeherder] wlach:1318474 > mozilla:master As I said in the PR, I flagged a couple things for you to double-check. But overall no major concerns. Thanks again for doing this! :)
Attachment #8811959 - Flags: review?(cdawson) → review+
Commits pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/9f604fa323db1d1d2e859c85b2c01810f7f781b7 Bug 1318474 - Switch API's and some internal consumers to use Django ORM for job data Still *ingesting* into datasource for now https://github.com/mozilla/treeherder/commit/1abbe58211c2933aad30757163b48565542bf679 Bug 1318474 - Remove obsolete e10s-special case handling in perf etl
Comment on attachment 8815854 [details] [treeherder] wlach:1318474-jobnote-deletion-fix > mozilla:master Could you have a look at this Cam? It's a fairly simple fix, but we need it to be applied before we can promote what's on master to production.
Attachment #8815854 - Flags: review?(cdawson)
Attachment #8815854 - Flags: review?(cdawson) → review+
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/3721af655041ea09795081307add6fbaa7a12a19 Bug 1318474 - Fix job note deletion (#2008) We were always resetting the failure classification to an invalid value when the last job note was deleted for a job, but we didn't notice it before because we didn't have foreign key validation.
Still need to add "not null" constraints to the metadata columns, now that we've transferred over the data. After that, I think we can resolve this particular bug.
Depends on: 1322067
Attached file SQL for disabling null keys (deleted) —
The last part of this (disabling null keys) I am applying manually, as Django can't apply this migration without generating errors. Here's the SQL that is being applied.
(In reply to William Lachance (:wlach) from comment #18) > The last part of this (disabling null keys) I am applying manually, as > Django can't apply this migration without generating errors. Here's the SQL > that is being applied. Could you elaborate? :-) Looking at the SQL I'm presuming it's due to the need to skip the expensive foreign key checks? Or if it's something else I'm happy to fix upstream Django bugs.
(In reply to Ed Morley [:emorley] from comment #19) > (In reply to William Lachance (:wlach) from comment #18) > > The last part of this (disabling null keys) I am applying manually, as > > Django can't apply this migration without generating errors. Here's the SQL > > that is being applied. > > Could you elaborate? :-) > > Looking at the SQL I'm presuming it's due to the need to skip the expensive > foreign key checks? Or if it's something else I'm happy to fix upstream > Django bugs. It's a limitation of innodb, that it apparently can't handle inserts during an alter table operation (which I think we might be seeing because of Django's get_or_create logic): " When running an online DDL operation, the thread that runs the ALTER TABLE statement will apply an “online log” of DML operations that were run concurrently on the same table from other connection threads. When the DML operations are applied, it is possible to encounter a duplicate key entry error (ERROR 1062 (23000): Duplicate entry), even if the duplicate entry is only temporary and would be reverted by a later entry in the “online log”. This is similar to the idea of a foreign key constraint check in InnoDB in which constraints must hold during a transaction." https://dev.mysql.com/doc/refman/5.6/en/innodb-create-index-limitations.html I've been working on documenting these gotchas, will file a bug about it tomorrow. PostgreSQL keeps on looking better.
Comment on attachment 8821279 [details] [treeherder] mozilla:1318474-use-global-id > mozilla:master Hey Cam, could you take a look at this one? It's the last externally facing change as part of the datasource removal. After this is applied, all API's should return and use ORM id's instead of datasource ones (there's a backward compatibility shim for old logviewer urls that we might have in orangefactor, etc.) and we can remove the rest of datasource at our leisure.
Attachment #8821279 - Flags: review?(cdawson)
Depends on: 1325540
Comment on attachment 8821279 [details] [treeherder] mozilla:1318474-use-global-id > mozilla:master This looks good to me. :)
Attachment #8821279 - Flags: review?(cdawson) → review+
Commits pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/82b3ea713fc96592489bc5b1eb03047ecea0a80b Bug 1318474 - Make jobs model take job ORM objects as input https://github.com/mozilla/treeherder/commit/32cfd2b6599860c6a9015d3db994b29e60c223cc Bug 1318474 - Make web api's use ORM ids as input and output https://github.com/mozilla/treeherder/commit/041f481a2cedd9c07c0cfc4dc9374c2eae0d0c94 Bug 1318474 - Make sure old logviewer links still work Unfortunately there are many older links to the logviewer with datasource ids floating around. Support them by looking for jobs with the old datasource id and the project inside the logviewer. If there's a match, then look up the log information for that job instead.
The PR referenced in comment 25 pretty much wraps up this bug, but I'll wait until after the holidays to ask for review.
Comment on attachment 8822275 [details] [treeherder] wlach:1318474-remove-jobmodel-read-methods > mozilla:master Hey Cam, we're almost done with these, I promise. :) This stops us from writing to datasource at all, paving the way for its outright removal.
Attachment #8822275 - Flags: review?(cdawson)
Depends on: 1328453
Attachment #8822275 - Flags: review?(cdawson) → review+
Blocks: 1328985
Attached file raw sql to apply (deleted) —
Here's the raw sql that should be applied to avoid django migrations issues with the last PR (note only the alter table statement for `job` requires pausing job ingestion)
Commits pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/a4a19007c0dff001c0c77d73f60c2da2abb0a2f8 Bug 1318474 - Fix SETA tests to not rely on datasource https://github.com/mozilla/treeherder/commit/901f6288a57947459f4d0c61c9dfbb3c85474712 Bug 1318474 - Rewrite unit tests to only look for ORM representation https://github.com/mozilla/treeherder/commit/34ac762fd60cb10547d7c343425f9a96d4ab633c Bug 1318474 - Remove a call to datasource jobs from autoclassify code https://github.com/mozilla/treeherder/commit/f4a319b2cf4f69e7a0eca85b4742e4ee128f43ae Bug 1318474 - Actually remove datasource calls from jobs model https://github.com/mozilla/treeherder/commit/f6407b74935bfebba234ea752c891334d756c956 Bug 1318474 - Stop writing to datasource altogether for jobs data The last thing we were using it for were cycling data and giving an id numbering for jobs, and we can easily replace those with calls to the ORM.
Blocks: 1329002
Applied all SQL changes except for the jobs table one to stage, will do rest tomorrow morning (requires a tree closure).
(In reply to William Lachance (:wlach) from comment #32) > Applied all SQL changes except for the jobs table one to stage, will do rest > tomorrow morning (requires a tree closure). s/stage/production/ :)
(In reply to William Lachance (:wlach) from comment #32) > Applied all SQL changes except for the jobs table one to stage, will do rest > tomorrow morning (requires a tree closure). trees are closed now
Schema change deployed on production, as was code. I think we're done here.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #8935894 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: