search

sasanrose / phpredmin

Yet another web interface for Redis
BSD 3-Clause "New" or "Revised" License
404 stars 96 forks source link

Multiple Redis servers #2

Closed asiminiceanu closed 10 years ago

asiminiceanu commented 11 years ago

It could be great if phpredmin could support multiple Redis servers in the configuration file. By default it could display the first server in the list and to have drop-down list close to the Actions ones to select the server latter on.

The cron should perform it's actions on all defined servers.

sasanrose commented 11 years ago

Great idea. Will implement that shortly

eugef commented 10 years ago

Hi @sasanrose, any update on this feature? Do you have any plans to implement this?

sasanrose commented 10 years ago

Hi @eugef Yeah, surely. I have plans for a lot of new features including this one. But unfortunately I am too busy these days working and studying.

eugef commented 10 years ago

Hi @sasanrose, i would like to help you with this feature. What is the best way to do it? I can fork phpredmin and then make pull request.

sasanrose commented 10 years ago

That would be great. I think the best way is to have an array of servers in config file. But it should support old config files too. Then we can handle redis servers similar to databases in code.

eugef commented 10 years ago

Hi @sasanrose, i fully agree with you about config. But i also want allow end-user to select servers/db on the fly through interface.

So i plan to add left column with list of all configured servers. For selected server user will see all available DBs and can select one of them (pretty similar to phpMyAdmin).

Also i want allow user to select different servers/db in separate browser's tabs. For this we need to store selected server/db not in session, but in get parameter or as a part of url. For example: phpredmin.tld/index.php/keys/search/s0/db1, where s0 - first redis server from config.php, db1 - second database on that server

sasanrose commented 10 years ago

That's a good idea. But I think it might face two problems. First, are you going to send DB and Server in every request? I don't think that would be a wise idea. Second, you also need controllers that run through command line. For example for generating statistics and also bulk action workers.

eugef commented 10 years ago

Yes, each request will contain Server and DB ids - this is the only way to allow open two different DBs is separate tabs and work with them. I don't see any issues with this approach.

Of course all CLI controllers should also be modified to get Server and DB ids as parameters. If no params are specified - first server from config file will be used.

sasanrose commented 10 years ago

Actually the approach is not bad at all (And I like the idea of considering a default value for server). However, I am worried about how you are going to implement this in code. Because I believe this will result in a messy code. My I ask about your plan to implement this (in code)?

eugef commented 10 years ago

I have made quick overview of the code and you are using session to store DB selected by user.

Raw idea - is to create some singleton model (lets call it DBConfig) that will store selected server and DB id. Then i will replace all occupancies of using session object with DBConfig.

DBConfig will read server and DB id from GET parameters or load default value from config file.

eugef commented 10 years ago

Hi @sasanrose, here is initial version of multi server support https://github.com/eugef/phpredmin/tree/multiserver

I haven't fully tested it (for example i haven't tested cron and gearman functionality), but all main features in UI work for me.

Your feedback is welcome.

I tried not to make too much changes, but because the whole code is tightly coupled and all dependencies are hardcoded, i had to make some refactoring.

eugef commented 10 years ago

Hi @sasanrose, i have added multi server support for statistics functionality https://github.com/eugef/phpredmin/tree/multiserver

Your feedback is highly appreciated!

sasanrose commented 10 years ago

Hi @eugef

I am really sorry for delay. Here is a diff (Please ignore config.php's diff): (Updated)

diff --git a/config.php b/config.php
index 9291b01..ddbc587 100644
--- a/config.php
+++ b/config.php
@@ -28,7 +28,7 @@ $config = Array(
             ),
             Array(
                 'host'     => '127.0.0.1',
-                'port'     => '6379',
+                'port'     => '63791',
                 'password' => Null,
                 'database' => 0
             ),
diff --git a/controllers/gearman.php b/controllers/gearman.php
index e45cde7..5edf103 100644
--- a/controllers/gearman.php
+++ b/controllers/gearman.php
@@ -16,26 +16,28 @@ class Gearman_Controller extends Controller

     public function deleteKeys($job)
     {
-        $data = $job->workload();
+        $data   = $job->workload();
+        $key    = $data['key'];
+        $server = $data['server'];

-        Log::factory()->write(Log::INFO, "Try to delete: {$data}", 'Gearman');
+        Log::factory()->write(Log::INFO, "Try to delete: {$key}", 'Gearman');

-        $keys  = $this->db->keys($data);
+        $keys  = $this->db->keys($key);
         $count = count($keys);

-        $this->db->set("phpredmin:deletecount:{$data}", $count);
-        $this->db->del("phpredmin:deleted:{$data}");
-        $this->db->del("phpredmin:requests:{$data}");
+        $this->db->set("phpredmin:{$server}:deletecount:{$key}", $count);
+        $this->db->del("phpredmin:{$server}:deleted:{$key}");
+        $this->db->del("phpredmin:{$server}:requests:{$key}");

         foreach ($keys as $key) {
             if ($this->db->delete($key) !== False) {
-                $this->db->incrBy("phpredmin:deleted:{$data}", 1);
-                $this->db->expireAt("phpredmin:deleted:{$data}", strtotime('+10 minutes'));
+                $this->db->incrBy("phpredmin:{$server}:deleted:{$key}", 1);
+                $this->db->expireAt("phpredmin:{$server}:deleted:{$key}", strtotime('+10 minutes'));
             } else
                 Log::factory()->write(Log::INFO, "Unable to delete {$key}", 'Gearman');
         }

-        $this->db->del("phpredmin:deletecount:{$data}");
+        $this->db->del("phpredmin:{$server}:deletecount:{$key}");
     }

     public function moveKeys($job)
diff --git a/controllers/keys.php b/controllers/keys.php
index 3f5ae54..4678608 100644
--- a/controllers/keys.php
+++ b/controllers/keys.php
@@ -113,7 +113,7 @@ class Keys_Controller extends Controller
                 $client = new GearmanClient();

                 $client->addServer($config['gearman']['host'], $config['gearman']['port']);
-                $client->doBackground('delete_keys', $key);
+                $client->doBackground('delete_keys', array('key' => $key, 'server' => $this->app->current['serverId']));
             }
         }
     }
diff --git a/libraries/db.php b/libraries/db.php
index b189dbd..5faf882 100644
--- a/libraries/db.php
+++ b/libraries/db.php
@@ -3,7 +3,7 @@ final class Db
 {
     protected static $_instances = Array();

-    public static function factory($driver = Null, $config)
+    public static function factory($driver = Null, $config = Array())
     {
         $driver = isset($driver) ? $driver : App::instance()->config['database']['driver'];

diff --git a/libraries/drivers/db/redis.php b/libraries/drivers/db/redis.php
index d94e562..d8eca65 100644
--- a/libraries/drivers/db/redis.php
+++ b/libraries/drivers/db/redis.php
@@ -2,13 +2,13 @@
 class RedisDb extends Redis
 {
     public function __construct($config) {
-        
+
         $this->connect($config['host'], $config['port']);
         $this->select($config['database']);

         if (isset($config['password'])) {
             $this->auth($config['password']);
-        }    
+        }
     }

     public function changeDB($db) {
diff --git a/libraries/drivers/template/php.php b/libraries/drivers/template/php.php
index e56ad10..229bb2d 100644
--- a/libraries/drivers/template/php.php
+++ b/libraries/drivers/template/php.php
@@ -20,13 +20,7 @@ class PhpTemplate

     public function render($__view, $data = Array())
     {
-        echo $this->renderPartial(
-            App::instance()->config['default_layout'], 
-            array(
-                'content' => $this->renderPartial($__view, $data),
-                'navigation' => $this->renderPartial('navigation'),
-            )
-        );
+        echo $this->renderPartial(App::instance()->config['default_layout'], array('content' => $this->renderPartial($__view, $data)));
     }

     public function renderPartial($__view, $__data = Array())
diff --git a/libraries/router.php b/libraries/router.php
index 3c18f4d..12825e9 100644
--- a/libraries/router.php
+++ b/libraries/router.php
@@ -28,7 +28,7 @@ final class Router
     protected function parse()
     {
         $this->method   = $_SERVER['REQUEST_METHOD'];
-        $this->protocol = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS']) ? 'https' : 'http'; 
+        $this->protocol = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS']) ? 'https' : 'http';
         $this->host     = $_SERVER['HTTP_HOST'];
         $this->baseUrl  = $this->protocol.'://'.$this->host;
         $this->url      = $this->protocol.'://'.$this->host.$_SERVER['SCRIPT_NAME'];
@@ -64,10 +64,10 @@ final class Router

         if (!$this->action = array_shift($this->_params))
             $this->action = App::instance()->config['default_action'];
-        
+
         if (!$this->serverId = array_shift($this->_params))
             $this->serverId = 0;
-        
+
         if (!$this->dbId = array_shift($this->_params))
             $this->dbId = 0;
     }
@@ -101,7 +101,7 @@ final class Router
             );
             if (method_exists($controller, $method)) {
                 call_user_func_array(array($controller, $method), $this->_params);
-            }    
+            }

             return;
         }
diff --git a/models/stats.php b/models/stats.php
index 2042ca3..7c00030 100644
--- a/models/stats.php
+++ b/models/stats.php
@@ -2,34 +2,43 @@

 class Stats_Model extends Model
 {
+    protected $server;
+
+    public function __construct($config)
+    {
+        parent::__construct($config);
+
+        $this->server = $this->app->current['serverId'];
+    }
+
     public function resetStats($info)
     {
-        $this->db->del('phpredmin:memory');
-        $this->db->del('phpredmin:connections');
-        $this->db->del('phpredmin:commands');
-        $this->db->del('phpredmin:expired_keys');
-        $this->db->del('phpredmin:hits');
-        $this->db->del('phpredmin:misses');
-        $this->db->del('phpredmin:clients');
-        $this->db->del('phpredmin:user_cpu');
-        $this->db->del('phpredmin:system_cpu');
+        $this->db->del("phpredmin:{$this->server}:memory");
+        $this->db->del("phpredmin:{$this->server}:connections");
+        $this->db->del("phpredmin:{$this->server}:commands");
+        $this->db->del("phpredmin:{$this->server}:expired_keys");
+        $this->db->del("phpredmin:{$this->server}:hits");
+        $this->db->del("phpredmin:{$this->server}:misses");
+        $this->db->del("phpredmin:{$this->server}:clients");
+        $this->db->del("phpredmin:{$this->server}:user_cpu");
+        $this->db->del("phpredmin:{$this->server}:system_cpu");

         foreach ($this->infoModel->getDbs($info) as $db)
             if (preg_match('/^keys=([0-9]+),expires=([0-9]+)$/', $info["db{$db}"], $matches)) {
-                $this->db->del("phpredmin:db{$db}:keys");
-                $this->db->del("phpredmin:db{$db}:expired_keys");
+                $this->db->del("phpredmin:{$this->server}:db{$db}:keys");
+                $this->db->del("phpredmin:{$this->server}:db{$db}:expired_keys");
             }
     }

     public function addKey($key, $value, $time)
     {
-        $this->db->zAdd("phpredmin:{$key}", $time, $value);
+        $this->db->zAdd("phpredmin:{$this->server}:{$key}", $time, $value);
     }

     public function getKeys($key, $from, $to)
     {
         $results = Array();
-        $keys    = $this->db->changeDB(0)->zRevRangeByScore("phpredmin:{$key}", $to, $from, Array('withscores' => True));
+        $keys    = $this->db->changeDB(0)->zRevRangeByScore("phpredmin:{$this->server}:{$key}", $to, $from, Array('withscores' => True));

         foreach ($keys as $key => $value) {
             $results[] = array($value, $key);
diff --git a/views/layout.php b/views/layout.php
index faae959..419896a 100644
--- a/views/layout.php
+++ b/views/layout.php
@@ -183,7 +183,7 @@
         </div>
         <div class="row">
             <div class="span2">
-                <?= $this->navigation ?>
+                <?= $this->renderPartial('navigation') ?>
             </div>
             <div class="span10">
                 <?= $this->content ?>
diff --git a/views/welcome/stats.php b/views/welcome/stats.php
index f47a747..e61ea1b 100644
--- a/views/welcome/stats.php
+++ b/views/welcome/stats.php
@@ -76,7 +76,7 @@
         $.each(methods, function(index, method) {
             $('#'+method+'_chart').empty();
             $.ajax({
-                url: '<?=$this->router->url?>/stats/<?= $this->app->current['serverId'] . '/' . $this->app->current['database'] ?>/'+method,
+                url: '<?=$this->router->url?>/stats/'+method+'/<?= $this->app->current['serverId'] . '/' . $this->app->current['database'] ?>',
                 dataType: 'json',
                 data: 'from='+from+'&to='+to,
                 success: function(data) {
eugef commented 10 years ago

Hi @sasan, thanks for review.

For now - i see only one issue with Stats_Model. My idea was to save statistics for each server in DB that belongs to that server. Because i wan't each server to work independently and store statistics internally.

That is why we don't need serverId in statistic keys.

sasanrose commented 10 years ago

The patch that i sent to you fixes the statistics functionality and does what you want. Also some some of the changes where unnecessary. For example there was no need to change php template.

eugef commented 10 years ago

Regarding statistics functionality - you have added serverId to the keys names. As i mentioned above - it is not necessary because each server stores its statistic data in its own database.

As serverId is just an index in config array - it can be anytime changed in config.php and server will "loose its statistics" because serverId was changed.

If we are not using serverId in the keys names then we don't have such an issue.

The same should go for Gearman - deleted keys should be stored on each server separately.

eugef commented 10 years ago

Hi @sasanrose I have applied you patches for the following files:

Following patches were not applied:

Latest code is here https://github.com/eugef/phpredmin/tree/multiserver

Gearman functionality i will check later.

sasanrose commented 10 years ago

@eugef I agree about the statistics and gearman, but about libraries/db.php i think it should be optional. Because it's a factory class and it is not just for Redis

eugef commented 10 years ago

Hi @sasanrose, because db is a factory class - $config should be required. Both Mysql and Redis requires host and port to create connection.

Old version of db factory didn't allow to create several connections to the same DB type but with different host/port (because host/port are not used to crate unique connection id). That is why i have added host and port as required params.

sasanrose commented 10 years ago

Hi @eugef Actually this is just a web panel for redis, and there is not going to be a MySQL or other DBMS support. Therefor, neither mine nor your concerns are important here. But i suggest to change the order of arguments here, because first argument is optional (can be null). I think it's better to put the optional argument before the mandatory one

eugef commented 10 years ago

@sasanrose, do you mean change

Db::factory($driver = Null, $config)

to

Db::factory($config, $driver = Null)

?

eugef commented 10 years ago

Hello @sasanrose,

I have implemented multiple server support for bulk delete, please review my changes https://github.com/eugef/phpredmin/commit/63c74d32c4660f12517c3b1deca6f2ca6195cab5

sasanrose commented 10 years ago

Hello @eugef Yes i think this way would be better but you need to change everywhere that we call this. Also I tested the bulk delete support and everything works fine. Thanks

eugef commented 10 years ago

Hello @sasanrose, I have committed following changes:

I think my branch is ready to be merged into main master branch :)

eugef commented 10 years ago

Hello @sasanrose

I have done some small improvements in user interface:

sasanrose commented 10 years ago

Wow @eugef You nailed it man, you are getting so active. I have to catchup ;) I tested the mentioned changes everything seems working great. May be this is time to merge your changes into main phpredmin repository

eugef commented 10 years ago

There are still some small issues with statistics, i will try to fix them in nearest week or two

eugef commented 10 years ago

Hello @sasanrose

I have finally fixed all issues and created pull request https://github.com/sasanrose/phpredmin/pull/20

sasanrose commented 10 years ago

Thanks to @eugef this feature is now available

asiminiceanu commented 10 years ago

Good work @eugef and @sasanrose :+1: