Page MenuHomePhabricator

Add support for querying feed transactions by author and date range
ClosedPublic

Authored by epriestley on Tue, May 21, 12:25 PM.

Details

Summary

Depends on D20531. Ref T13294. Enable finding raw transactions in particular date ranges or with particular authors.

Test Plan

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Tue, May 21, 12:25 PM
epriestley requested review of this revision.Tue, May 21, 12:27 PM
amckinley accepted this revision.Tue, May 21, 6:40 PM
amckinley added inline comments.
src/applications/feed/query/PhabricatorFeedTransactionQuery.php
93

Really this is for the previous diff, but shouldn't we fire all of these in parallel?

This revision is now accepted and ready to land.Tue, May 21, 6:40 PM
epriestley added inline comments.Tue, May 21, 7:37 PM
src/applications/feed/query/PhabricatorFeedTransactionQuery.php
93

Running queries in parallel is theoretically possible, but requires mysqli_async(), which not all installs have and we don't really use for anything, and execute() does a lot of other work so it would need to be unravelled.

So "yes", but we have no practical ability to do parallel queries right now and this probably isn't a particularly strong case for developing that capability since I expect it to be rarely used for answering pretty narrow audit-related questions.

If some other use case arose to motivate development of parallel query capabilities, coming back here and converting this would probably make sense, although we do get some benefit of doing these serially by shrinking the date window progressively below as results come back. After a bit we should have a fairly cheap query (examining a very small time window).

There's currently no dateCreated key on the transaction tables so this probably doesn't actually help us very much today, but if these queries are slow in practice we could likely add that key to largely fix the problem (it would just trigger a bit of a migration mess).

I expect this page may be "kind of slowish" on real large installs but likely not explosively bad, but I guess we'll see.