From a89cef8e396c31e7fd84be12257d93503bff51d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 20 May 2012 14:46:01 -0700 Subject: [PATCH] Remove PHID database, add Harbormaster database Summary: - We currently write every PHID we generate to a table. This was motivated by two concerns: - **Understanding Data**: At Facebook, the data was sometimes kind of a mess. You could look at a random user in the ID tool and see 9000 assocs with random binary data attached to them, pointing at a zillion other objects with no idea how any of it got there. I originally created this table to have a canonical source of truth about PHID basics, at least. In practice, our data model has been really tidy and consistent, and we don't use any of the auxiliary data in this table (or even write it). The handle abstraction is powerful and covers essentially all of the useful data in the app, and we have human-readable types in the keys. So I don't think we have a real need here, and this table isn't serving it if we do. - **Uniqueness**: With a unique key, we can be sure they're unique, even if we get astronomically unlucky and get a collision. But every table we use them in has a unique key anyway. So we actually get pretty much nothing here, except maybe some vague guarantee that we won't reallocate a key later if the original object is deleted. But it's hard to imagine any install will ever have a collision, given that the key space is 36^20 per object type. - We also currently use PHIDs and Users in tests sometimes. This is silly and can break (see D2461). - Drop the PHID database. - Introduce a "Harbormaster" database (the eventual CI tool, after Drydock). - Add a scratch table to the Harbormaster database for doing unit test meta-tests. - Now, PHID generation does no writes, and unit tests are isolated from the application. - @csilvers: This should slightly improve the performance of the large query-bound tail in D2457. Test Plan: Ran unit tests. Ran storage upgrade. Reviewers: btrahan, vrana, jungejason Reviewed By: btrahan CC: csilvers, aran, nh, edward Differential Revision: https://secure.phabricator.com/D2466 --- resources/sql/patches/phiddrop.sql | 1 + resources/sql/patches/testdatabase.sql | 7 +++ src/__phutil_library_map__.php | 7 +-- .../storage/base/HarbormasterDAO.php} | 4 +- .../storage/base/__init__.php | 2 +- .../scratchtable/HarbormasterScratchTable.php | 29 +++++++++++++ .../storage/scratchtable/__init__.php | 12 ++++++ .../phid/storage/phid/PhabricatorPHID.php | 18 ++------ .../phid/storage/phid/__init__.php | 3 -- .../PhabricatorBuiltinPatchList.php | 16 +++++-- ...rontIsolatedDatabaseConnectionTestCase.php | 43 ++++++++----------- .../isolated/__tests__/__init__.php | 2 +- ...AphrontMySQLDatabaseConnectionTestCase.php | 2 +- .../connection/mysql/__tests__/__init__.php | 2 +- .../dao/__tests__/LiskFixtureTestCase.php | 36 +++++++--------- src/storage/lisk/dao/__tests__/__init__.php | 2 +- 16 files changed, 109 insertions(+), 77 deletions(-) create mode 100644 resources/sql/patches/phiddrop.sql create mode 100644 resources/sql/patches/testdatabase.sql rename src/applications/{phid/storage/base/PhabricatorPHIDDAO.php => harbormaster/storage/base/HarbormasterDAO.php} (88%) rename src/applications/{phid => harbormaster}/storage/base/__init__.php (78%) create mode 100644 src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php create mode 100644 src/applications/harbormaster/storage/scratchtable/__init__.php diff --git a/resources/sql/patches/phiddrop.sql b/resources/sql/patches/phiddrop.sql new file mode 100644 index 0000000000..53c95e5c20 --- /dev/null +++ b/resources/sql/patches/phiddrop.sql @@ -0,0 +1 @@ +DROP DATABASE IF EXISTS {$NAMESPACE}_phid; \ No newline at end of file diff --git a/resources/sql/patches/testdatabase.sql b/resources/sql/patches/testdatabase.sql new file mode 100644 index 0000000000..6c9d8c1d42 --- /dev/null +++ b/resources/sql/patches/testdatabase.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_scratchtable ( + `id` int unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY, + `data` varchar(64) NOT NULL collate utf8_bin, + `dateCreated` int unsigned NOT NULL, + `dateModified` int unsigned NOT NULL, + KEY (data) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 82764cb217..0777f48dce 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -423,6 +423,8 @@ phutil_register_library_map(array( 'DrydockResourceStatus' => 'applications/drydock/constants/resourcestatus', 'DrydockSSHCommandInterface' => 'applications/drydock/interface/command/ssh', 'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/base', + 'HarbormasterDAO' => 'applications/harbormaster/storage/base', + 'HarbormasterScratchTable' => 'applications/harbormaster/storage/scratchtable', 'HeraldAction' => 'applications/herald/storage/action', 'HeraldActionConfig' => 'applications/herald/config/action', 'HeraldApplyTranscript' => 'applications/herald/storage/transcript/apply', @@ -783,7 +785,6 @@ phutil_register_library_map(array( 'PhabricatorPHID' => 'applications/phid/storage/phid', 'PhabricatorPHIDConstants' => 'applications/phid/constants', 'PhabricatorPHIDController' => 'applications/phid/controller/base', - 'PhabricatorPHIDDAO' => 'applications/phid/storage/base', 'PhabricatorPHIDLookupController' => 'applications/phid/controller/lookup', 'PhabricatorPaste' => 'applications/paste/storage/paste', 'PhabricatorPasteController' => 'applications/paste/controller/base', @@ -1411,6 +1412,8 @@ phutil_register_library_map(array( 'DrydockResourceStatus' => 'DrydockConstants', 'DrydockSSHCommandInterface' => 'DrydockCommandInterface', 'DrydockWebrootInterface' => 'DrydockInterface', + 'HarbormasterDAO' => 'PhabricatorLiskDAO', + 'HarbormasterScratchTable' => 'HarbormasterDAO', 'HeraldAction' => 'HeraldDAO', 'HeraldApplyTranscript' => 'HeraldDAO', 'HeraldCommitAdapter' => 'HeraldObjectAdapter', @@ -1689,9 +1692,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersOwner' => 'PhabricatorOwnersDAO', 'PhabricatorOwnersPackage' => 'PhabricatorOwnersDAO', 'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO', - 'PhabricatorPHID' => 'PhabricatorPHIDDAO', 'PhabricatorPHIDController' => 'PhabricatorController', - 'PhabricatorPHIDDAO' => 'PhabricatorLiskDAO', 'PhabricatorPHIDLookupController' => 'PhabricatorPHIDController', 'PhabricatorPaste' => 'PhabricatorPasteDAO', 'PhabricatorPasteController' => 'PhabricatorController', diff --git a/src/applications/phid/storage/base/PhabricatorPHIDDAO.php b/src/applications/harbormaster/storage/base/HarbormasterDAO.php similarity index 88% rename from src/applications/phid/storage/base/PhabricatorPHIDDAO.php rename to src/applications/harbormaster/storage/base/HarbormasterDAO.php index 01c88caa52..e48d0cb12c 100644 --- a/src/applications/phid/storage/base/PhabricatorPHIDDAO.php +++ b/src/applications/harbormaster/storage/base/HarbormasterDAO.php @@ -16,10 +16,10 @@ * limitations under the License. */ -abstract class PhabricatorPHIDDAO extends PhabricatorLiskDAO { +abstract class HarbormasterDAO extends PhabricatorLiskDAO { public function getApplicationName() { - return 'phid'; + return 'harbormaster'; } } diff --git a/src/applications/phid/storage/base/__init__.php b/src/applications/harbormaster/storage/base/__init__.php similarity index 78% rename from src/applications/phid/storage/base/__init__.php rename to src/applications/harbormaster/storage/base/__init__.php index 5c42a7b15d..6c2fe6963a 100644 --- a/src/applications/phid/storage/base/__init__.php +++ b/src/applications/harbormaster/storage/base/__init__.php @@ -9,4 +9,4 @@ phutil_require_module('phabricator', 'applications/base/storage/lisk'); -phutil_require_source('PhabricatorPHIDDAO.php'); +phutil_require_source('HarbormasterDAO.php'); diff --git a/src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php b/src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php new file mode 100644 index 0000000000..a0bf76940b --- /dev/null +++ b/src/applications/harbormaster/storage/scratchtable/HarbormasterScratchTable.php @@ -0,0 +1,29 @@ +setPHIDType($type); - $phid_rec->setOwnerPHID($owner); - $phid_rec->setParentPHID($parent); - $phid_rec->setPHID($phid); - $phid_rec->save(); - - return $phid; + return 'PHID-'.$type.'-'.$uniq; } } diff --git a/src/applications/phid/storage/phid/__init__.php b/src/applications/phid/storage/phid/__init__.php index 11b016fe93..46963fc681 100644 --- a/src/applications/phid/storage/phid/__init__.php +++ b/src/applications/phid/storage/phid/__init__.php @@ -6,10 +6,7 @@ -phutil_require_module('phabricator', 'applications/phid/storage/base'); - phutil_require_module('phutil', 'filesystem'); -phutil_require_module('phutil', 'utils'); phutil_require_source('PhabricatorPHID.php'); diff --git a/src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php b/src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php index a599aabd2b..41456e08c8 100644 --- a/src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/setup/sql/phabricator/PhabricatorBuiltinPatchList.php @@ -83,6 +83,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'db', 'name' => 'flag', ), + 'db.harbormaster' => array( + 'type' => 'db', + 'name' => 'harbormaster', + ), 'db.herald' => array( 'type' => 'db', 'name' => 'herald', @@ -115,10 +119,6 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'db', 'name' => 'phame', ), - 'db.phid' => array( - 'type' => 'db', - 'name' => 'phid', - ), 'db.phriction' => array( 'type' => 'db', 'name' => 'phriction', @@ -871,6 +871,14 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('emailtableremove.sql'), ), + 'phiddrop.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('phiddrop.sql'), + ), + 'testdatabase.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('testdatabase.sql'), + ), ); } diff --git a/src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php b/src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php index eed20ebff5..391260eebc 100644 --- a/src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php +++ b/src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php @@ -29,15 +29,10 @@ final class AphrontIsolatedDatabaseConnectionTestCase } public function testIsolation() { - $conn = $this->newIsolatedConnection(); - $test_phid = $this->generateTestPHID(); - + // This will fail if the connection isn't isolated. queryfx( - $conn, - 'INSERT INTO phabricator_phid.phid (phid) VALUES (%s)', - $test_phid); - - $this->assertNoSuchPHID($test_phid); + $this->newIsolatedConnection(), + 'INSERT INVALID SYNTAX'); } public function testInsertGeneratesID() { @@ -116,23 +111,21 @@ final class AphrontIsolatedDatabaseConnectionTestCase public function testTransactionRollback() { $check = array(); - $phid = new PhabricatorPHID(); + $phid = new HarbormasterScratchTable(); $phid->openTransaction(); for ($ii = 0; $ii < 3; $ii++) { - $test_phid = $this->generateTestPHID(); + $key = $this->generateTestData(); - $obj = new PhabricatorPHID(); - $obj->setPHID($test_phid); - $obj->setPHIDType('TEST'); - $obj->setOwnerPHID('PHID-UNIT-!!!!'); + $obj = new HarbormasterScratchTable(); + $obj->setData($key); $obj->save(); - $check[] = $test_phid; + $check[] = $key; } $phid->killTransaction(); - foreach ($check as $test_phid) { - $this->assertNoSuchPHID($test_phid); + foreach ($check as $key) { + $this->assertNoSuchRow($key); } } @@ -141,19 +134,19 @@ final class AphrontIsolatedDatabaseConnectionTestCase return new AphrontIsolatedDatabaseConnection($config); } - private function generateTestPHID() { - return 'PHID-TEST-'.Filesystem::readRandomCharacters(20); + private function generateTestData() { + return Filesystem::readRandomCharacters(20); } - private function assertNoSuchPHID($phid) { + private function assertNoSuchRow($data) { try { - $real_phid = id(new PhabricatorPHID())->loadOneWhere( - 'phid = %s', - $phid); + $row = id(new HarbormasterScratchTable())->loadOneWhere( + 'data = %s', + $data); $this->assertEqual( null, - $real_phid, - 'Expect fake PHID to exist only in isolation.'); + $row, + 'Expect fake row to exist only in isolation.'); } catch (AphrontQueryConnectionException $ex) { // If we can't connect to the database, conclude that the isolated // connection actually is isolated. Philosophically, this perhaps allows diff --git a/src/storage/connection/isolated/__tests__/__init__.php b/src/storage/connection/isolated/__tests__/__init__.php index 01b64ad096..8756fd32ca 100644 --- a/src/storage/connection/isolated/__tests__/__init__.php +++ b/src/storage/connection/isolated/__tests__/__init__.php @@ -6,7 +6,7 @@ -phutil_require_module('phabricator', 'applications/phid/storage/phid'); +phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable'); phutil_require_module('phabricator', 'infrastructure/testing/testcase'); phutil_require_module('phabricator', 'storage/connection/isolated'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php b/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php index bca29e9310..1ee16af444 100644 --- a/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php +++ b/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php @@ -27,7 +27,7 @@ final class AphrontMySQLDatabaseConnectionTestCase } public function testConnectionFailures() { - $conn = id(new PhabricatorPHID())->establishConnection('r'); + $conn = id(new HarbormasterScratchTable())->establishConnection('r'); queryfx($conn, 'SELECT 1'); diff --git a/src/storage/connection/mysql/__tests__/__init__.php b/src/storage/connection/mysql/__tests__/__init__.php index aa3e537fbf..51b48813af 100644 --- a/src/storage/connection/mysql/__tests__/__init__.php +++ b/src/storage/connection/mysql/__tests__/__init__.php @@ -6,7 +6,7 @@ -phutil_require_module('phabricator', 'applications/phid/storage/phid'); +phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable'); phutil_require_module('phabricator', 'infrastructure/testing/testcase'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php b/src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php index 91479460cc..750d997837 100644 --- a/src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php +++ b/src/storage/lisk/dao/__tests__/LiskFixtureTestCase.php @@ -29,38 +29,35 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { // If the user from either test persists, the other test will fail. $this->assertEqual( 0, - count(id(new PhabricatorUser())->loadAll())); + count(id(new HarbormasterScratchTable())->loadAll())); - id(new PhabricatorUser()) - ->setUserName('alincoln') - ->setRealName('Abraham Lincoln') + id(new HarbormasterScratchTable()) + ->setData('alincoln') ->save(); } public function testTransactionalIsolation2of2() { $this->assertEqual( 0, - count(id(new PhabricatorUser())->loadAll())); + count(id(new HarbormasterScratchTable())->loadAll())); - id(new PhabricatorUser()) - ->setUserName('ugrant') - ->setRealName('Ulysses S. Grant') + id(new HarbormasterScratchTable()) + ->setData('ugrant') ->save(); } public function testFixturesBasicallyWork() { $this->assertEqual( 0, - count(id(new PhabricatorUser())->loadAll())); + count(id(new HarbormasterScratchTable())->loadAll())); - id(new PhabricatorUser()) - ->setUserName('gwashington') - ->setRealName('George Washington') + id(new HarbormasterScratchTable()) + ->setData('gwashington') ->save(); $this->assertEqual( 1, - count(id(new PhabricatorUser())->loadAll())); + count(id(new HarbormasterScratchTable())->loadAll())); } public function testReadableTransactions() { @@ -70,18 +67,17 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { LiskDAO::endIsolateAllLiskEffectsToTransactions(); try { - $phid = 'PHID-TEST-'.Filesystem::readRandomCharacters(32); + $data = Filesystem::readRandomCharacters(32); - $obj = new PhabricatorPHID(); + $obj = new HarbormasterScratchTable(); $obj->openTransaction(); - $obj->setPHID($phid); - $obj->setPHIDType('TEST'); + $obj->setData($data); $obj->save(); - $loaded = id(new PhabricatorPHID())->loadOneWhere( - 'phid = %s', - $phid); + $loaded = id(new HarbormasterScratchTable())->loadOneWhere( + 'data = %s', + $data); $obj->killTransaction(); diff --git a/src/storage/lisk/dao/__tests__/__init__.php b/src/storage/lisk/dao/__tests__/__init__.php index 21703c010e..b221a85ac5 100644 --- a/src/storage/lisk/dao/__tests__/__init__.php +++ b/src/storage/lisk/dao/__tests__/__init__.php @@ -6,7 +6,7 @@ -phutil_require_module('phabricator', 'applications/people/storage/user'); +phutil_require_module('phabricator', 'applications/harbormaster/storage/scratchtable'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); phutil_require_module('phabricator', 'infrastructure/testing/testcase'); phutil_require_module('phabricator', 'storage/lisk/dao');