Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make udp optionals #36

Closed
soullivaneuh opened this issue Jun 5, 2016 · 9 comments
Closed

Make udp optionals #36

soullivaneuh opened this issue Jun 5, 2016 · 9 comments

Comments

@soullivaneuh
Copy link
Collaborator

soullivaneuh commented Jun 5, 2016

Currently, both http and upd client/database instances are created, even if we don't needed.

Maybe we could make it optional with the config:

influx_db:
    connections:
        telegraf:
            host: '%influxdb_host%'
            database: '%influxdb_telegraf_db%'
            udp: false # Disable UDP

Both udp and http port would be false by default and a configuration validation could check if at least one of them is enabled.

Edit: Only udp will be optional: #36 (comment)

What do you think?

@soullivaneuh soullivaneuh changed the title Make udp or http optional Make udp or http optionals Jun 5, 2016
@soullivaneuh
Copy link
Collaborator Author

Doing this is BC break, so I think this should be done before 2.0.0 release.

@Algatux
Copy link
Owner

Algatux commented Jun 5, 2016

I don't like to much enabling or disabling base services of the bundle, instead of enabling/disabling them. what if we make them lazy ?

http://symfony.com/doc/current/components/dependency_injection/lazy_services.html

@soullivaneuh
Copy link
Collaborator Author

Will take a look.

@soullivaneuh
Copy link
Collaborator Author

This will be not useful at all, because connection definition require Client instanciation:

$client = new Client($host, $httpPort, $user, $password);

So even if the services are lazy loaded, we will have useless Client instances.

So the only way is the way I described on my issue AFAIK.

If you prefer to keep both udp and http services, then close the issue.

@Algatux
Copy link
Owner

Algatux commented Jun 6, 2016

It will be confusing if users make use of events.
Can we deny use of soecific event if related client is not enabled?

@soullivaneuh
Copy link
Collaborator Author

Can we deny use of soecific event if related client is not enabled?

What do you mean by deny? I think the listener could simply ignore them.

If you think it's too complex, so we can just leave it as is.

@Algatux
Copy link
Owner

Algatux commented Jun 6, 2016

Or the listener may trow an exception ...

@soullivaneuh
Copy link
Collaborator Author

Or the listener may trow an exception ...

That's an idea. 👍

@soullivaneuh
Copy link
Collaborator Author

@Algatux Since #42, the http connection is required too.

I think the http connection should always be provided, and the udp optional.

This is basically the same way on the core library: https://github.com/influxdata/influxdb-php#writing-data-using-udp

An http instance is create first, then you change the driver for the UDP one.

@soullivaneuh soullivaneuh changed the title Make udp or http optionals Make udp optionals Jun 6, 2016
@soullivaneuh soullivaneuh mentioned this issue Jun 6, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants