Add support for messagepack #734
Conversation
lovasoa
force-pushed
the
master
branch
3 times, most recently
from
edd52c0 to
2034bd1
Compare
|
@aviau Is it ok now ? |
|
@aviau I think this is ready for merging. |
|
I will look at this as soon as I have time, thanks! |
|
@aviau : Did you have the time to look at it ? |
|
Can someone please review that ? |
|
@aviau Can you find a few minutes to review that and merge it if it's ok ? |
|
For the love of god someone please check this in! |
|
@xginn8: Maybe you can review this ? |
|
What shall I do to have this reviewed ? This is a PR that gives a 25% speedup to the library and fixes multiple issues, and no one seems to be interested. @russorat maybe ? |
|
@lovasoa oh wow, this is awesome. Sorry for taking so long here. I'll find someone to review this. While I do that, could you sign the CLA agreement https://influxdata.com/community/cla/ ? |
|
Hooray ! Thank you. |
|
So sorry for the delay @lovasoa, but we REALLY appreciate your contribution here. I'm going to look around at some other PRs and build up a release candidate shortly. |
|
When do you think you can make a new release with that code ? |
|
I know I've kicked the can a few times, but let's target this week for a release. Will get this sorted. |
* Add support for messagepack * Remove unnecessary blank line Fixes https://github.com/influxdata/influxdb-python/pull/734/files/57daf8ccd5027c796a2fd3934b8e88d3982d300e#r302769403 * Small code reorganization * Small code reorganization Fixes influxdata#734 (comment)
|
the hard lock on 0.6.1 prevents this from being integrated with other solutions. |
|
Oh yes, the dependency requirement should be updated. @prometheanfire, you should make a pull request. But good luck for getting it merged: mine took 5 months before someone finally looked at it. |
|
ya, that seems to be common for oneliners. The amount of time it takes to merge is inversely proportional to the size of the patch. |
|
Mine (#734) wasn't a one-liner, although it wasn't huge either.
|
|
true, I've had the same issue with pins or capping with others though (salt and boto off the top of my head). |
- Update the work of PR influxdata#734 to correctly interpret negative timestamps
- Update the work of PR influxdata#734 to correctly interpret negative timestamps - Update unit test for pre-unix epoch date with negative timestamp
| return self._read_chunked_response(response) | ||
|
|
||
| data = response.json() | ||
| data = response._msgpack |
There was a problem hiding this comment.
is msgpack meant to ignore chunked answers?
Description
This pull request implements fetching data from InfluxDB using msgpack serialization instead of JSON. This fixes issues related to serialization ambiguities in JSON, and makes database queries faster. It does not break compatibility with older influxdb versions that do not support msgpack.
Related issues
Closes #733
Fixes #665
Fixes #715
Fixes #625
Performance
Here is a performance comparison before (JSON) and after (msgpack) this pull request, for a simple
SELECT *request on a measurement with 2 tags and three fields of types: integer, float and string:Using msgpack gives a ~25% speedup.