Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Delay driver instantiation until it's actually needed. #68

Merged
merged 1 commit into from
May 17, 2017

Conversation

pprkut
Copy link
Contributor

@pprkut pprkut commented Oct 25, 2016

This essentially removes the dependency on Guzzle if you use
an alternative driver implementation. "Essentially" since the tests
are pretty hard-wired to Guzzle, but runtime-wise it wouldn't be
needed anymore.

This essentially removes the dependency on Guzzle if you use
an alternative driver implementation.
@gianarb
Copy link
Contributor

gianarb commented Oct 26, 2016

This change is totally good but I am not sure that this kind of change can fit in the current stable version of the SDK.

I am in my mind a version 2 totally agnostic about guzzle but I am not really on it right know.

@pprkut can you show me your use case?
Thanks

@pprkut
Copy link
Contributor Author

pprkut commented Oct 26, 2016

My use case is move-backend@d167936, a driver using https://github.com/rmccue/Requests instead of Guzzle.
I was planning to file a pull request for that too, but it depends on this one and #69, so I wanted to get those in first.

Not sure how that would fit in with your planning for v2 though.

@gianarb
Copy link
Contributor

gianarb commented Oct 26, 2016

#69 will be merged and released soon.. Just time to have a refactoring and make a release.

This one requires a little bit of time more sorry

@pprkut
Copy link
Contributor Author

pprkut commented Oct 26, 2016

That's totally fine. We have our fork to use in the meantime. But the general idea would still be to get this (and the Requests driver) upstream at some point :)

@tuupola
Copy link
Contributor

tuupola commented Oct 26, 2016

IMO it would be a good idea to decouple next major version influxdb-php from the actual http driver implementation. HTTPlug would be perfect for this. Only bad thing is that it does not support UDP.

@thecodeassassin
Copy link
Collaborator

thecodeassassin commented May 17, 2017

@pprkut @gianarb is this still something to consider?

@pprkut
Copy link
Contributor Author

pprkut commented May 17, 2017

I would still like to have this :)
FWIW, we're using this in our production environments for half a year now and it's working well for us

@thecodeassassin
Copy link
Collaborator

@pprkut the thing is it's not really friendly in terms of performance when you have a huge number of queries since you are creating an object every time. Can i suggest you move driver to a static property of the client class (and check if it's already instantiated). If you make this change and check the tests i can merge it.

@pprkut
Copy link
Contributor Author

pprkut commented May 17, 2017

The behavior of how many objects are created didn't change. The only change is when it is instantiated. This code won't create a new driver object every time you call query, it will only do it the first time. Afterwards $this->driver is not null, and thus getDriver will return the already existing driver object. And if you have multiple instances of the Client class, you can just use setDriver to always set the same driver object.

@thecodeassassin
Copy link
Collaborator

My bad, you are right. LGTM

@thecodeassassin thecodeassassin merged commit c938497 into influxdata:master May 17, 2017
@thecodeassassin
Copy link
Collaborator

@pprkut thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants