1134916 - Jobs API returns data in wrong order (should be sorted by push timestamp)

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect, P1)

Description

[Johannes Pfrang [:johnp]]

The job API of .allizom.org returns data in the format from old to new, whereas at .mozilla.org, it is returned from new to old.

The most likely cause is bug 1097090 which removed several pieces of sorting code and ORDER BY statements from SQL statements during the re-factoring.

Comment 1

[Johannes Pfrang [:johnp]]

Here's the request from the similar_jobs panel from .mozilla.org (correctly ordered):

> https://treeherder.mozilla.org/api/project/mozilla-central/jobs/?build_platform_id=45&count=21&full=false&job_type_id=123&offset=0&option_collection_hash=102210fe594ee9b33d82058545b1ed14f4c8206e

And there's the same request from .allizom.org:

> https://treeherder.allizom.org/api/project/mozilla-central/jobs/?build_platform_id=18&count=21&full=false&job_type_id=49&offset=0&option_collection_hash=102210fe594ee9b33d82058545b1ed14f4c8206e

Comment 2

[Johannes Pfrang [:johnp]]

From what I see the issue was introduced in jobs.json here:

https://github.com/mozilla/treeherder-service/commit/119407ad8ecaa787abd629891645b8ae38796098

When the new "get_job_list" SQL was written, the 'ORDER BY' statement from the original "get_job_list" was lost and there was no possibility added for specifying an ordering through the API.

So the fix should either be to add back the missing 'ORDER BY' statement, or to add an URL query parameter that defaults to the correct ordering.

Comment 3

[Ed Morley [:emorley]]

Good spot - thank you for noticing this and filing the bug! :-)

Comment 4

[Ed Morley [:emorley]]

Attachment: Make jobs API return jobs sorted by push_timestamp again

<•edmorley> camd: I've added the order by, and added a test for it, however other tests now break, since they expect the original order. Are we happy that sorting the list of jobs by push_timestamp descending (which is what the similar jobs panel is ok for all other consumers (including other parts of the UI)? If so, I'll just update the other tests. If not, I
<•edmorley> guess we'll have to pass in an order param
<•edmorley> I can only begin to think of what else uses this api, I've not really dug into this area much yet
<•edmorley> PR with failing tests opened for reference ^

Comment 5

[Ed Morley [:emorley]]

The transcript didn't copy properly.

My question to you was whether we should add the sort (which the similar jobs panel needs) and fix the tests it breaks, or whether we think other consumers (both the UI and others) don't want the jobs sorted? (Given that before, we had two queries, one sorted and one not, and so some consumers would be used to unsorted).

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 8567233 [details] [review]
Make jobs API return jobs sorted by push_timestamp again

PR updated, ready for review.
Hopefully this is the last thing that blocks the deployment! :-)

Comment 7

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-service

https://github.com/mozilla/treeherder-service/commit/d6400a41152a89b109a883ac009602b35eba3a9c
Bug 1134916 - Make jobs API return jobs sorted by push_timestamp again

Bug 1097090 combined get_job_list and get_job_list_full, but the two
queries were actually subtly different. The former had an ORDER BY
push_timestamp, which was lost when they were combined. This means jobs
displayed in the similar jobs panel are from the past, and not the most
recent jobs of the same type.

The get_job_list query also sorted on platform, however I don't believe
this is necessary, so I've not added it back in here.

https://github.com/mozilla/treeherder-service/commit/6fca7fe4ac9728941d5b5ca07e217c6b867ce31b
Bug 1134916 - Fix tests that depend on jobs API response order

Tests that are not aimed at the jobs API should not be dependant on the
order of jobs returned by get_job_list().

* test_tbpl.py does not even need to use get_job_list() since the only
  accessed property is the job_id, which we are better off hard-coding.
* test_note_apy.py should use the job_id found earlier in the test,
  rather than hard-coding a wrong value.
* In test_bug_job_map_api.py, there is no ORDER BY clause for the stored
  get_bug_job_map_list query. The current test only happens to pass
  since the bug_job_map table currently uses the InnoDB engine, which
  default to the order of the primary key. Were our test environment and
  production bug_job_map tables to use different engines, the behaviour
  would silently change, so it seems wrong for the test to give the
  illusion of a guaranteed order. If in the future we wanted to give
  such a guarantee, we should add an ORDER BY to the
  get_bug_job_map_list query & update the test accordingly.

Comment 8

[Ed Morley [:emorley]]

(In reply to Treeherder Bugbot from comment #7)
> The get_job_list query also sorted on platform, however I don't believe
> this is necessary, so I've not added it back in here.

Mauro, when you're back could you confirm this part is correct? :-)

Comment 9

[Mauro Doglio [:mdoglio]]

Yes, that sounds correct.