Remove support for 12 character ('short') SHA revisions
Categories
(Tree Management :: Treeherder: Data Ingestion, defect, P3)
Tracking
(Not tracked)
People
(Reporter: wlach, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files, 2 obsolete files)
I'd like us to consider unsupporting submitting <40 character revisions. Is there any reason we shouldn't? We have a bunch of code associated with keeping the short and long revision fields in sync that I'd like not to have to rewrite when moving stuff into the main db (bug 1306674).
Reporter | ||
Comment 1•8 years ago
|
||
Do we have any submitters who are giving us 12 character revisions but not 40? (or 12 then 40 later?)
Comment 2•8 years ago
|
||
There are still some job types (eg l10n jobs) that use the 12 character SHA. Though: (a) I think we should just give them a hard deadline and then leave it up to them (b) Even if we have to support 12 character jobs, perhaps we can remove support from eg push ingestion / the resultset API etc New Relic attribute recording is likely a good way forwards here.
Updated•8 years ago
|
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Ed Morley [:emorley] from comment #2) > There are still some job types (eg l10n jobs) that use the 12 character SHA. > ... Ok, that's annoying but I guess we'll just live with it in the short term. At least the business logic to handle this will be easier to write with the Django ORM.
Comment 4•8 years ago
|
||
I think we should at least identify which jobs still are using 12 characters and file bugs for them. The l10n jobs (and I imagine any/most other culprits) are in builds-4hr - I'll have a shot at analysing now.
Comment 5•8 years ago
|
||
Even builds-pending/builds-running contain 12 character SHAs :-( Builds-pending: { "pending": { "try": { "1a855f92cc45": [ { "submitted_at": 1475064907, "id": 125865687, "buildername": "Windows 10 64-bit try debug test mochitest-gl-1" }, Builds-running (at least it has the full SHA in there): { "running": { "autoland": { "cdf154426881": [ { "submitted_at": 1475263520, "buildername": "Android armv7 API 15+ autoland build", "start_time": 1475263723, "number": 320, "claimed_by_name": "buildbot-master74.bb.releng.usw2.mozilla.com:/builds/buildbot/build1/master", "request_ids": [ 126258716 ], "last_heartbeat": 1475264067, "id": 127438343, "revision": "cdf154426881032934224b0ad60bc366afebff41" } ],
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
Main candidates are: * PGO builds plus their corresponding tests * Various jobs run on comm-* (though these are < tier-3) * Desktop+Android l10n nightlies
Updated•8 years ago
|
Comment 8•8 years ago
|
||
Some places are still using 12 character SHAs in UI URLs too. eg on: https://hg.mozilla.org/mozilla-central/rev/af6e01b8574b "[default view]" -> https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=87cd291d2db6 "[failures only]" -> https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=87cd291d2db6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception (the perfherder links on that page use 40 character SHAs however) Unfortunately we can't use server logs to see how many people are doing this, since the fragment part of the URL is not sent in the HTTP request. Think we'll have to resort to GitHub code search etc.
Comment 9•8 years ago
|
||
Will, I still think we can simplify things on our side somewhat, ie we can remove support for 12 character SHAs from: * Resultsets ingestion via our API * Resultsets ingestion via json-pushes polling * Resultsets ingestion via Pulse (if not already done; I can't find the schema for it) * [DONE] Jobs ingestion via Pulse (see pulse-job.yml) * The `revision` table (since I'm pretty sure for buildbot job ingestion we only need short_revision on the result_set table, right?) Similarly, whilst the revision hash removal (bug 1257602) is blocked on some older repos (though we should double-check which - given B2G EOL and ESRs only run taskcluster as tier <1 so can probably just have those jobs hidden), we can probably remove support from many other places.
Comment 10•8 years ago
|
||
I mean we could even remove `short_revision` from the `result_set` table if we were prepared to do LIKE queries against `long_revision` for these legacy buildbot job types and/or UI URLs that are still using 12 character SHAs.
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
Checking again shows many cases fixed. Left are some PGO jobs that are instances of bug 1313274, plus aurora instances of bug 1306722 (needs backporting).
Comment 13•7 years ago
|
||
Now that bug 1306718 is deployed, builds-{pending,running} use the full 40 character SHAs. This just leaves the self-serve triggered builds (eg sheriffs triggering PGO runs but only pasting in 12 character SHAs), which is bug 1313274. An alternative and possibly quicker fix for that is to make it easier to copy the full SHA from Treeherder (bug 1340265).
Comment 14•7 years ago
|
||
I'm not going to wait on bug 1313274 or bug 1340265 any longer. Let's just do this.
Comment 15•6 years ago
|
||
Updated•6 years ago
|
Comment 16•5 years ago
|
||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
After deploying, New Relic reported instances of:
treeherder.model.models:MultipleObjectsReturned: get() returned more than one Push -- it returned 2!
and
treeherder.model.models:DoesNotExist: Push matching query does not exist.
I believe this is since repository
is no longer in a few of the queries, eg:
https://github.com/mozilla/treeherder/pull/4534/files#diff-d1c18844859a68f2ac3f6aa32603cdbeL54
I've rolled back the prod deploy for now - master will need a fix before we deploy again.
Comment 18•5 years ago
|
||
Comment 20•5 years ago
|
||
The 2nd PR landed in:
https://github.com/mozilla/treeherder/commit/bcb0bc2d7ca34e9ca780a4b737f7f7896c395802
Comment 21•5 years ago
|
||
Actually, reopening since the API still supports 12 character revisions and their usage is being tracked via:
https://github.com/mozilla/treeherder/pull/4534/files#diff-d1c18844859a68f2ac3f6aa32603cdbeR272
The API now does the optimal MySQL query if provided with a 40 character SHA, however the fallback should be removed to prevent people from accidentally relying on it (eg when writing tools that comment on bugs etc).
When doing so it may be worth making the API explicitly return a JSON error message and HTTP 400 for SHAs that are not 40 characters (and also ensuring it's bubbled up to the jobs-view and Perfherder UIs) to prevent confusing UX from 404s from seemingly valid SHAs.
Comment 22•5 years ago
|
||
This would be good to get done. But I'm not going to get to it anytime soon.
Comment 23•1 year ago
|
||
cleaning up old bugs we haven't fixed
Description
•