Fix several migration issues with the Task/Counter patch
Summary:
People hit three issues with D3914:
  - As per T2059, we applied a schema change from a `.php` patch, which currently does not work if you use a different user to make schema changes than for normal use.
    - Since the change in question is idempotent, just move it to a `.sql` patch. We'll follow up in T2059 and fix it properly.
  - Rogue daemons at several installs used old code (expecting autoincrement) to insert into the new table (no autoincrement), thereby creating tasks with ID 0.
    - Rename the table so they'll fail.
    - This also makes the code a little more consistent.
  - Some installs now have tasks with ID 0.
    - Use checks against null rather than against 0 so we can process these tasks.
The major issues this fixes are the schema upgrade failure in T2059, and the infinite loops in T2072 and elsewhere.
This isn't really a fully statisfactory fix. I'll discuss some next steps in T2072.
Test Plan: Created new tasks via MetaMTA/Differential. Ran tasks with `phd debug taskmaster`. Inserted a task 0 and verified it ran and archived correctly.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2072, T2059
Differential Revision: https://secure.phabricator.com/D3973
			
			
This commit is contained in:
		
							
								
								
									
										8
									
								
								resources/sql/patches/liskcounters-task.sql
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										8
									
								
								resources/sql/patches/liskcounters-task.sql
									
									
									
									
									
										Normal file
									
								
							| @@ -0,0 +1,8 @@ | ||||
| ALTER TABLE `{$NAMESPACE}_worker`.worker_task | ||||
|   CHANGE id id INT UNSIGNED NOT NULL; | ||||
|  | ||||
| RENAME TABLE `{$NAMESPACE}_worker`.worker_task | ||||
|   TO `{$NAMESPACE}_worker`.worker_activetask; | ||||
|  | ||||
| UPDATE `{$NAMESPACE}_worker`.lisk_counter | ||||
|   SET counterName = 'worker_activetask' WHERE counterName = 'worker_task'; | ||||
| @@ -6,6 +6,8 @@ | ||||
| $active_table = new PhabricatorWorkerActiveTask(); | ||||
| $archive_table = new PhabricatorWorkerArchiveTask(); | ||||
|  | ||||
| $old_table = 'worker_task'; | ||||
|  | ||||
| $conn_w = $active_table->establishConnection('w'); | ||||
|  | ||||
| $active_auto = head(queryfx_one( | ||||
| @@ -13,12 +15,12 @@ $active_auto = head(queryfx_one( | ||||
|   'SELECT auto_increment FROM information_schema.tables | ||||
|     WHERE table_name = %s | ||||
|     AND table_schema = DATABASE()', | ||||
|   $active_table->getTableName())); | ||||
|   $old_table)); | ||||
|  | ||||
| $active_max = head(queryfx_one( | ||||
|   $conn_w, | ||||
|   'SELECT MAX(id) FROM %T', | ||||
|   $active_table->getTableName())); | ||||
|   $old_table)); | ||||
|  | ||||
| $archive_max = head(queryfx_one( | ||||
|   $conn_w, | ||||
| @@ -33,12 +35,6 @@ queryfx( | ||||
|     VALUES (%s, %d) | ||||
|     ON DUPLICATE KEY UPDATE counterValue = %d', | ||||
|   LiskDAO::COUNTER_TABLE_NAME, | ||||
|   $active_table->getTableName(), | ||||
|   $old_table, | ||||
|   $initial_counter + 1, | ||||
|   $initial_counter + 1); | ||||
|  | ||||
| // Drop AUTO_INCREMENT from the ID column. | ||||
| queryfx( | ||||
|   $conn_w, | ||||
|   'ALTER TABLE %T CHANGE id id INT UNSIGNED NOT NULL', | ||||
|   $active_table->getTableName()); | ||||
|   | ||||
| @@ -5,10 +5,6 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { | ||||
|   private $serverTime; | ||||
|   private $localTime; | ||||
|  | ||||
|   public function getTableName() { | ||||
|     return 'worker_task'; | ||||
|   } | ||||
|  | ||||
|   public function getConfiguration() { | ||||
|     return array( | ||||
|       self::CONFIG_IDS => self::IDS_COUNTER, | ||||
| @@ -71,7 +67,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { | ||||
|   } | ||||
|  | ||||
|   public function archiveTask($result, $duration) { | ||||
|     if (!$this->getID()) { | ||||
|     if ($this->getID() === null) { | ||||
|       throw new Exception( | ||||
|         "Attempting to archive a task which hasn't been save()d!"); | ||||
|     } | ||||
|   | ||||
| @@ -10,7 +10,7 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { | ||||
|   protected $result; | ||||
|  | ||||
|   public function save() { | ||||
|     if (!$this->getID()) { | ||||
|     if ($this->getID() === null) { | ||||
|       throw new Exception( | ||||
|         "Trying to archive a task with no ID."); | ||||
|     } | ||||
|   | ||||
| @@ -1028,6 +1028,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { | ||||
|         'type'    => 'sql', | ||||
|         'name'    => $this->getPatchPath('repository-lint.sql'), | ||||
|       ), | ||||
|       'liskcounters-task.sql' => array( | ||||
|         'type'    => 'sql', | ||||
|         'name'    => $this->getPatchPath('liskcounters-task.sql'), | ||||
|       ), | ||||
|     ); | ||||
|   } | ||||
|  | ||||
|   | ||||
		Reference in New Issue
	
	Block a user
	 epriestley
					epriestley