Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I don't believe the query does that at the moment.
db_query("UPDATE {pift_test}
SET status = %d
WHERE type = %d
AND id IN (
SELECT f.fid
FROM {files} f
LEFT JOIN {upload} u
ON f.fid = u.fid
LEFT JOIN {comment_upload} cu
ON f.fid = cu.fid
JOIN {project_issues} pi
ON " . $clause . "
JOIN {pift_project} p
ON pi.pid = p.pid
JOIN {project_release_nodes} r
ON pi.rid = r.nid
JOIN {node} n
ON r.nid = n.nid
JOIN {term_node} t
ON (n.vid = t.vid AND t.tid IN (" . db_placeholders($api_versions, 'int') . "))
WHERE pi.sid IN (" . db_placeholders($sids, 'int') . ")
)
AND status > %d
AND last_tested < %d",
array_merge(array(PIFT_STATUS_QUEUE, PIFT_TYPE_FILE), $api_versions, $sids,
array(PIFT_STATUS_SENT, $retest_time)));
Comment | File | Size | Author |
---|---|---|---|
#28 | 675460_28.patch | 2.38 KB | jthorson |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedComment #2
boombatower CreditAttribution: boombatower commentedUsing the following test code I was able to determine that the query does infact affect all patches on an issues instead of just the last.
I am not really sure how to fix this since we cannot merge the node and comment table. Otherwise, have to do a select and then update inside a loop which doesn't sound good.
Comment #3
boombatower CreditAttribution: boombatower commentedCurrent query with values:
Untested, but this should be what we are looking at:
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's just add the nid to pift_test, and be done with that:
Comment #6
webchickSubscriiiiibe!
Comment #7
catchSubscribing, I assume this is why patch re-testing is switched off at the moment.
Comment #8
rfayComment #9
jthorson CreditAttribution: jthorson commented.
Comment #10
jthorson CreditAttribution: jthorson commentedSimilar historical issue located at #635334: Patches are not being re-tested automatically.
Comment #11
jthorson CreditAttribution: jthorson commentedAttached patch contains a new 'Select' query for validation purposes before implementing the actual update.
Note: This patch is dependent on #1348958-6: Add 'nid' column to pift_test landing first.
Comment #12
jthorson CreditAttribution: jthorson commentedNeeds some optimization, but this appears to work:Comment #13
jthorson CreditAttribution: jthorson commentedOoops. Wrong version. Getting late. :(
Comment #14
jthorson CreditAttribution: jthorson commentedAgain (with a select instead of update).
Comment #15
jthorson CreditAttribution: jthorson commentedEdit: Had an issue with a named constant.
Comment #16
jthorson CreditAttribution: jthorson commentedNew candidate for testing/validation, but hard-coded to only work once, and trigger a very small number of retests (instead of flooding us with 1100+ tests, which is what would happen if we put this live without a careful launch strategy).
Comment #17
jthorson CreditAttribution: jthorson commentedLast patch was a non-starter.
This one works. :)
But because enabling this could totally overload the testbots for a few days, I've left some safety checks in place. To actually enable re-testing using this patch, comment out the
$retest_time = 1;
line near the beginning of pift.cron.inc and reinstate the line before it.We'll need to come up with a plan for a 'staged' retesting until we've cleared the backlog ... my thought was that enabling for "drush pift-cron", but not for pift_cron() would allow us to manually trigger batch retests as needed.
Comment #18
jthorson CreditAttribution: jthorson commentedDeployment Options:
1. Add a new drush command which accepts an argument 'X' and processes 'X' retests upon execution; leaving the 'retest' variable at 'disabled' until the backlog is cleared ... manually queueing during slow periods, and setting it up as a jenkins job once we know a reasonable value for 'X'.
2. Add the current 'queue depth' to the communication from PIFR to PIFT, and queue x retests based on queue depth triggers.
TODO: Update the 'retest interval' options to include weeks/months.
Comment #19
boombatower CreditAttribution: boombatower commentedNot a 100% sure what we did originally when this feature was turned on. Can we simply have a drush command for retesting that we then call on a different interval say twice a day and it only sends 30-40 tests until we are caught up? I mean having the the retests run an a slower interval is probably better anyway since they are lower priority.
Comment #20
jthorson CreditAttribution: jthorson commentedYup ... that was the thought (Option 1).
The second thought was more for documentation's sake, and would probably be a longer term feature ... added it here so that I didn't forget it the next morning.
Comment #21
sunStale commented out debug code?
Can you run an EXPLAIN on this query and post the results?
That is, because this query looks very expensive and potentially very long-running to me.
JOINs should always be explicit INNER JOIN or LEFT JOIN.
I've troubles to understand this comment. If I get the query correctly, then matching patches/tests are getting reset to be "queued"?
1) All SQL statement keywords, such as UPDATE and SET should be all-uppercase.
2) ORDER BY and LIMIT is seems bogus here.
A cron callback should not trigger dsm().
Comment #22
jthorson CreditAttribution: jthorson commentedUpdated for the D7 drupal.org upgrade ... D7 Project* changes made the query look worse.
Will begin testing this tomorrow.
Comment #23
jthorson CreditAttribution: jthorson commentedComment #24
jthorson CreditAttribution: jthorson commentedUpdated version.
Comment #25
jthorson CreditAttribution: jthorson commentedRelease candidate
Comment #26
jthorson CreditAttribution: jthorson commentedCommitted version.
Will be included in the next PIFT deploy, but the timeline for enabling the feature will be dependent on i) the number of outstanding RTBC retests, ii) confirming that the retest query doesn't negatively impact the drupal.org database, and iii) sorting out which option we use for actually triggering the retests.
Comment #27
jthorson CreditAttribution: jthorson commentedClose, but first tests on prod uncovered that the query needs a bit of refactoring.
Right now, it chooses the 'last passed file that has not already been sent' per RTBC issue, as opposed to the 'last passed file *if it* has not already been sent'.
Comment #28
jthorson CreditAttribution: jthorson commentedOkay, this looks better!
Comment #29
jthorson CreditAttribution: jthorson commentedOkay, did some more work on this, to try and see why it was negatively affecting drupal.org when I tried to enable it. I've now got pift-cron running only once per minute, and cleared out a test linking to a deleted file that was gumming things up ... and the PIFT logs look a lot more 'normal' now. There's a good chance that this query was trying to run twice concurrently.
The retest query itself looks like this:
With the EXPLAIN pastebinned at http://privatepaste.com/9b19b39c2b
So ... any mysql optimizers able to help improve this?
Comment #30
drummAdd
ORDER BY NULL
to the subquery:(SELECT MAX(pd2.test_id) AS maxid FROM pift_data pd2 GROUP BY nid ORDER BY NULL)
GROUP BY
implies a sort, which isn't too relevant in a subquery.Comment #31
drummIf any of the
WHERE
conditions onpd
can also be added topd2
in the subquery, that will reduce the number of rows examined.Otherwise, the query is okay, since it is not run concurrently.
Comment #32
jthorson CreditAttribution: jthorson commentedThe 'where' conditions need to be outside, as otherwise we get the last patch that meets the criteria, instead of getting the last patch and THEN seeing if it meets the criteria.
In any case, I've got a patch that restructures this into an EFQ to get the RTBC issues, and then uses php to filter out the last test. Will look at deploying for testing today.
Comment #33
jthorson CreditAttribution: jthorson commentedThe revamped patch (using EFQ to get issues, then loading all tests and sorting via PHP) is now live, via a new jenkins job.
The job will requeue up to 5 RTBC issues every 15 minutes. There are in the neighborhood of 400 RTBC patches given all of core and contrib, but 2/3 of these are D6/contrib tests which complete testing within just a couple minutes each; so they should not have a heavy impact on the queue. Core alone has 108 tests, which includes all of D6/D7/D8.