Algatux / influxdb-bundle

Bundle service integration of official influxdb/influxdb-php client
MIT License
24 stars 14 forks source link

Manage multiple connections #21

Closed soullivaneuh closed 8 years ago

soullivaneuh commented 8 years ago

Closes #8

To be merged for next major only.

The goal is to allow something like this: http://symfony.com/doc/master/bundles/DoctrineBundle/configuration.html#doctrine-dbal-configuration

Algatux commented 8 years ago

@Soullivaneuh, Must we wait for this one to release 2.0 ?

soullivaneuh commented 8 years ago

@Algatux I would prefer, yes. Because I'm not sure I can do it without BC breaks.

soullivaneuh commented 8 years ago

@Algatux This is not finished, I have to manage the listener.

I pushed an update to let you see the base changes.

soullivaneuh commented 8 years ago

Integration example:

$ ./docker-console app/console debug:container | grep influx
  algatux_influx_db.connection.default.http                                        InfluxDB\Database                                                                               
  algatux_influx_db.connection.default.udp                                         InfluxDB\Database                                                                               
  algatux_influx_db.connection.http                                                alias for "algatux_influx_db.connection.default.http"                                           
  algatux_influx_db.connection.udp                                                 alias for "algatux_influx_db.connection.default.udp"                                            
soullivaneuh commented 8 years ago

@Algatux I don't really know how to manage the listener with multiple connection.

When the event is dispatched, how to get the right connection service to write the points?

If you have any suggestion, I'm hearing! :wink:

Algatux commented 8 years ago

Thinking about. For now let write on default one

soullivaneuh commented 8 years ago

Thinking about. For now let write on default one

I found how to do, please take a look.

Algatux commented 8 years ago

@Soullivaneuh, I gave it a fast view, it's a good solution ... must review it again but great work 👏

soullivaneuh commented 8 years ago

@Algatux I can't understand this error: https://travis-ci.org/Algatux/influxdb-bundle/jobs/135430406#L363

Do you have any idea?

soullivaneuh commented 8 years ago

I have got another issue:

$ ./docker-console app/console debug:container --show-private | grep influx
  algatux_influx_db.client.influxdb.4444.8086.http                                                    InfluxDB\Client                                                                                 
  algatux_influx_db.client.influxdb.4444.8086.udp                                                     InfluxDB\Client                                                                                 
  algatux_influx_db.connection.awstats.http                                                           InfluxDB\Database                                                                               
  algatux_influx_db.connection.awstats.udp                                                            InfluxDB\Database                                                                               
  algatux_influx_db.connection.http                                                                   alias for "algatux_influx_db.connection.telegraf.http"                                          
  algatux_influx_db.connection.telegraf.http                                                          InfluxDB\Database                                                                               
  algatux_influx_db.connection.telegraf.udp                                                           InfluxDB\Database                                                                               
  algatux_influx_db.connection.udp                                                                    alias for "algatux_influx_db.connection.telegraf.udp"                                           
  algatux_influx_db.event_listener.awstats                                                            Algatux\InfluxDbBundle\Events\Listeners\InfluxDbEventListener                                   
  algatux_influx_db.event_listener.telegraf                                                           Algatux\InfluxDbBundle\Events\Listeners\InfluxDbEventListener                                   
$ ./docker-console app/console debug:container  | grep influx
  algatux_influx_db.connection.awstats.http                                        InfluxDB\Database                                                                               
  algatux_influx_db.connection.awstats.udp                                         InfluxDB\Database                                                                               
  algatux_influx_db.connection.http                                                alias for "algatux_influx_db.connection.telegraf.http"                                          
  algatux_influx_db.connection.telegraf.http                                       InfluxDB\Database                                                                               
  algatux_influx_db.connection.telegraf.udp                                        InfluxDB\Database                                                                               
  algatux_influx_db.connection.udp                                                 alias for "algatux_influx_db.connection.telegraf.udp"                                           
  algatux_influx_db.event_listener.awstats                                         Algatux\InfluxDbBundle\Events\Listeners\InfluxDbEventListener                                   
  algatux_influx_db.event_listener.telegraf                                        Algatux\InfluxDbBundle\Events\Listeners\InfluxDbEventListener                                   

So algatux_influx_db.client.influxdb.4444.8086.http and algatux_influx_db.client.influxdb.4444.8086.udp should not be accessible, right?

But if I do this from a controller:

dump($this->get('algatux_influx_db.client.influxdb.4444.8086.http'));

It works...

Algatux commented 8 years ago

Tried with : $definition->setPublic(false) ?

http://symfony.com/doc/2.8/components/dependency_injection/advanced.html

soullivaneuh commented 8 years ago

Tried with : $definition->setPublic(false) ?

Yeah already tried, change nothing. BTW, this property is already defined on the abstract client service.

soullivaneuh commented 8 years ago

From: http://symfony.com/doc/current/components/dependency_injection/advanced.html#marking-services-as-public-private

In some cases, a service only exists to be injected into another service and is not intended to be fetched directly from the container as shown above.

This may or may not work, depending on if the service could be inlined. Simply said: A service can be marked as private if you do not want to access it directly from your code.

So in a nutshell, the service is not private because it's used many times as a factory.

Possible solution: Instantiate client directly and do not defined them. This can be possible as the Database instance has the client on his constructor.

soullivaneuh commented 8 years ago

@Algatux I did it on another way.

I created a ConnectionFactory that will instantiate and store Client instances to avoid duplication.

The algatux_influx_db.connection_factory is still accessible even if it's private, but I marked the class as internal so the users are warned.

BTW, this fix my tests. :tada:

I still have to make some other tests and update the documentation. After that, we will be good! :+1:

soullivaneuh commented 8 years ago

Finished! \o/

soullivaneuh commented 8 years ago

About 2.0 release. This is as you want but I think we should solves remaining issues before: https://github.com/Algatux/influxdb-bundle/issues

Some can't be done on a BC way.

Algatux commented 8 years ago

Impressed! Very good work!

soullivaneuh commented 8 years ago

Thank you!