search

Algatux / influxdb-bundle

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

Refactor InfluxDbEventListener to use new system #30

Closed soullivaneuh closed 8 years ago

soullivaneuh commented 8 years ago

Closes #17.

All deprecated class and method usages are replaced.

Algatux commented 8 years ago

@Soullivaneuh the code looks a lot better, but theres a problem about the http calls done by the listener. The hold implementation worked to minimize the number of http calls grouping them by precision.

For example if in a request lyfecicle one user is dispatching 20 http deferred events with the same precision, with this implementation 20 http requests will be fired. with the old only one.

soullivaneuh commented 8 years ago

@Algatux Okay I'll take a look. Any suggestion?

Algatux commented 8 years ago

IMHO Similar to the old one we can use a service to store and merge the points by precision. (A service with a reusable interface to make future use of caches like memcached or redis will be good) This will be a statusfull service (not like it too mutch) if no cache is used

soullivaneuh commented 8 years ago

I would like to try something inside the listener.

Algatux commented 8 years ago

Make a proposition

soullivaneuh commented 8 years ago

@Algatux Done. I also make this class internal as this should not be used manually.

I don't know if the service should be private or no...

Algatux commented 8 years ago

Really really good work! Do you mean the listener ? if so yes

Algatux commented 8 years ago

Did you think abut wrapping database.http and udp into a service that gives access to them. I was wondering if it will be more ordered avoiding direct access to the other two services

soullivaneuh commented 8 years ago

Do you mean the listener ? if so yes

Will do this.

Did you think abut wrapping database.http and udp into a service that gives access to them.

Something like this?

$container->get('algatux_influx_db.database')->getHttp();

Not sure about that, this will add more complexity.

soullivaneuh commented 8 years ago

I can't make the listener private:

The service "algatux_influx_db.events_listeners.influx_db_event_listener" must be public as event listeners are lazy-loaded.

So the PR is RTM for me. :+1: