Posted by boombatower on January 6, 2010 at 5:26am
| Project: | Project issue file test |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
I don't believe the query does that at the moment.
<?php
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)));
?>
Comments
#1
#2
Using 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.
<?php
$api_versions = pift_core_api_versions();
$sids = variable_get('pift_status', array());
// $retest_time = time() - PIFT_RETEST * 20;
$retest_time = time();
// $retest_time = 0;
drupal_set_message(print_r($api_versions, TRUE));
drupal_set_message(print_r($sids, TRUE));
drupal_set_message($retest_time);
foreach (array('u.nid = pi.nid', 'cu.nid = pi.nid') as $clause) {
$result = db_query("SELECT *
FROM {pift_test}
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_TYPE_FILE), $api_versions, $sids,
array(PIFT_STATUS_SENT, $retest_time)));
while ($file = db_fetch_array($result)) {
drupal_set_message(print_r($file, TRUE));
}
}
?>
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.
#3
Current query with values:
SELECT *
FROM {pift_test}
WHERE type = 2
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 u.nid = pi.nid
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 (87, 103))
WHERE pi.sid IN (8, 14)
)
AND status > 2
AND last_tested < 1262816107
Untested, but this should be what we are looking at:
SELECT *
FROM
(
(
SELECT *
FROM {pift_test}
WHERE type = 2
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 u.nid = pi.nid
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 (87, 103))
WHERE pi.sid IN (8, 14)
)
AND status > 2
AND last_tested < 1262816107
)
UNION
(
SELECT *
FROM {pift_test}
WHERE type = 2
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 cu.nid = pi.nid
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 (87, 103))
WHERE pi.sid IN (8, 14)
)
AND status > 2
AND last_tested < 1262816107
)
ORDER BY fid DESC
) x
GROUP BY nid
#4
Let's just add the nid to pift_test, and be done with that:
SELECT nid, MAX(id) FROM {pift_test} WHERE status > 2 AND last_tested < 1262816107 GROUP BY nid#6
Subscriiiiibe!
#7
Subscribing, I assume this is why patch re-testing is switched off at the moment.
#8
#9
.
#10
Similar historical issue located at #635334: Patches are not being re-tested automatically.
#11
Attached 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.
#12
Needs some optimization, but this appears to work:#13
Ooops. Wrong version. Getting late. :(
#14
Again (with a select instead of update).
#15
Edit: Had an issue with a named constant.
#16
New 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).
#17
Last 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.
#18
Deployment 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.
#19
Not 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.
#20
Yup ... 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.
#21
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ //$retest_time = time() - PIFT_RETEST;
Stale commented out debug code?
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ // Get last file test from each nid which is in 'needs review' or 'rtbc'.
+ $result = db_query(
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.
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ JOIN {project_issues} pi ON pt.nid = pi.nid
+ JOIN {project_release_nodes} prn ON pi.rid = prn.nid
+ JOIN {node} n ON prn.nid = n.nid
+ JOIN {term_node} t
JOINs should always be explicit INNER JOIN or LEFT JOIN.
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ // Update status for tests with final results (Passed or Failed)
+ // and which have not been tested in the retest period.
I've troubles to understand this comment. If I get the query correctly, then matching patches/tests are getting reset to be "queued"?
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ "Update {pift_test} set status = %d
...
+ ORDER BY test_id ASC
+ LIMIT 50",
1) All SQL statement keywords, such as UPDATE and SET should be all-uppercase.
2) ORDER BY and LIMIT is seems bogus here.
+++ b/pift.cron.inc@@ -16,47 +16,47 @@ function pift_cron_retest() {
+ // TODO: Remove 'Limit 50' above and DSM() before enabling in pift.module!
+ drupal_set_message("Re-Tests Queued: " . db_affected_rows());
A cron callback should not trigger dsm().