Adding a command line parameter "-db-host" to optionally specify a re… #79
Conversation
There was a problem hiding this comment.
First its worth mentioning that this will make the benchmarks inaccurate, and shouldn't be used to measure HHVM performance. The unpredictability of the network accesses, will add variance.
It would be best if this worked for all frameworks. It seems like you only set it up for a couple.
base/PerfOptions.php
Outdated
| @@ -110,6 +110,8 @@ | |||
|
|
|||
| public bool $notBenchmarking = false; | |||
|
|
|||
| public ?string $dbHost; //The hostname/IP of server which hosts the database. | |||
There was a problem hiding this comment.
This doesn't need to be optionally null, and if it is null things will break.
targets/drupal7/Drupal7Target.php
Outdated
|
|
||
| $file = __DIR__.'/temp.php'; | ||
| $file_contents = file_get_contents($file); | ||
| $file_contents = preg_replace('/^.*\'host\' => \'.*$/m', ' \'host\' => \''.$this->options->dbHost.'\',', $file_contents ); |
There was a problem hiding this comment.
80 chars per line. This should happen everywhere.
targets/drupal7/Drupal7Target.php
Outdated
| @@ -31,6 +31,15 @@ public function install(): void { | |||
| ); | |||
| } | |||
|
|
|||
| copy('compress.zlib://' . __DIR__.'/settings.php.gz', __DIR__.'/temp.php'); | |||
There was a problem hiding this comment.
This section is modifying the files that get checked in. We don't want to do that if possible. This should modify the files after they are copied to the tmp directory used for the test.
Also consider putting a pattern in the settings.php file that you match for. Something like:
__DB_HOST__. You can find other parts of this project that use that style.
base/DatabaseInstaller.php
Outdated
| @@ -46,8 +48,8 @@ public function installDatabase(): bool { | |||
| $this->checkMySQLConnectionLimit(); | |||
| return false; | |||
| } | |||
|
|
|||
| $conn = mysql_connect('127.0.0.1', $db, $db); | |||
|
|
|||
There was a problem hiding this comment.
Try not to add whitespace at the end of lines.
| @@ -40,6 +40,11 @@ public function install(): void { | |||
| $cache_dir = $this->getSourceRoot().'/mw-cache'; | |||
| mkdir($cache_dir); | |||
|
|
|||
| $file = __DIR__.'/LocalSettings.php'; | |||
There was a problem hiding this comment.
Just like with drupal7 this should modify the file after it has been copied to the tmp directory where the test is going to be run from. (And use a pattern)
| $file = __DIR__.'/wp-config.php'; | ||
| $file_contents = file_get_contents($file); | ||
| $file_contents = preg_replace('/^.*DB_HOST.*$/m', 'define(\'DB_HOST\', \''.$this->options->dbHost.'\');', $file_contents ); | ||
| file_put_contents($file, $file_contents); |
There was a problem hiding this comment.
Again try using a pattern, and modify the temp copy not the checked in files.
base/PerfOptions.php
Outdated
| @@ -292,6 +295,10 @@ public function __construct(Vector<string> $argv) { | |||
|
|
|||
| $argTempDir = $this->getNullableString('temp-dir'); | |||
|
|
|||
| if(array_key_exists('db-host', $o)){ | |||
There was a problem hiding this comment.
This needs to be an option that requires --i-am-not-benchmarking. Use one of the getNullableString functions to make it so.
targets/drupal7/Drupal7Target.php
Outdated
| @@ -30,11 +30,16 @@ public function install(): void { | |||
| $this->getSourceRoot().'/sites/default', | |||
| ); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Avoid unneeded spaces here as well.
kevand900
force-pushed
the
remoteDB
branch
4 times, most recently
from
23ad5fe to
1bc23ee
Compare
|
Can you please review? Thanks! |
…mote machine to host the mysql database.
This option is to be used with "--i-am-not-benchmarking".
Changes were made in PerfOptions.php for the command line parsing, and DatabaseInstaller.php for installing the database.
Drupal7, Mediawiki, and Wordpress are the targets which support this feature presently.
--ON THE SERVER RUNNING MYSQL--
Edit my.cnf
$sudo vim /etc/mysql.cnf
Comment out '#skip-networking' if present:
Change to 'bind-address=0.0.0.0'
Restart the server:
$sudo service mariadb stop
$sudo service mariadb start
Create a user 'root' with IP of the machine running OSS:
MariaDB [(none)]>CREATE USER 'root'@'' IDENTIFIED BY 'root';
Grant Privileges to this user:
MariaDB [(none)]>GRANT ALL PRIVILEGES ON . TO 'root'@'' IDENTIFIED BY 'root' WITH GRANT OPTION;
MariaDB [(none)]>FLUSH PRIVILEGES;
Make sure there is a user named 'root'@'localhost.localdomain':
MariaDB [(none)]>SELECT User, Host FROM mysql.user;
+----------+-----------------------+
| User | Host |
+----------+-----------------------+
| root | 127.0.0.1 |
| wp_bench | 127.0.0.1 |
| root | ::1 |
| root | localhost |
| root | localhost.localdomain |
+----------+-----------------------+
Allow remote access through IPTables:
iptables -P INPUT ACCEPT
iptables -F
iptables -A INPUT -i lo -j ACCEPT
ON MACHINE RUNNING OSS
Test connection from machine running OSS:
$mysql -u root -proot -h
When running OSS, add the command line parameter "db-host=" along with "--i-am-not-benchmarking"