- Notifications
You must be signed in to change notification settings - Fork524
Fixes/msgpack default#783
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- raise proper exception when send_packet is called with invalid protocol- fix missing 'optional' flag for query() parameters- fix shadowed parameters response and filter- add extra msgpack test starting from json with int, float and string values- default to old behaviour for client (don't use msgpack by default - see failing test)
… is int.- retention policy creation : cast to int, adjust docs- client_test : some tests using@raises(Exception) were succeeding for the wrong reason(assertion in mock function): using self.assertRaises() instead.- test all functions with msgpack enabled and disabled (2 clients)
| else: | ||
| self._headers = { | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'text/plain' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Whytext/plain and notapplication/json ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You would have to ask the original implementor - this is the way it was implemented before moving the default to msgpack.
lovasoa commentedFeb 21, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What is this issue you identified ? |
| client_1 = InfluxDBClient('localhost', self.influxd_inst.http_port, | ||
| 'root', '', database='db', use_msgpack=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this test should be added. There is a bug in influxdb (influxdata/influxdb#8707) that causes the JSON response to be parsed as the wrong datatype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Like I said in the description :
The test test_write_points_mixed_type fails on purpose now because of a difference between the json and msgpack result returned by the server.
I can remove the defaulting to json and just make sure all tests run with both json and msgpack client.
I think users would want to be made aware of the consequences of defaulting to msgpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The consequences you are talking about is a bug that is now fixed with floats being misinterpreted as integers. I agree with you that this bugfix should be mentioned is the change log.
In any case, you don't want to havetest_write_points_mixed_type run with the json client, because it fails due to a bug in influxdb, not in this library.
| 'time': '2009-11-10T23:02:00Z', | ||
| "host": "server01", | ||
| "region": "us-west"}, | ||
| {'value': 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The fact that influxdb returns 1 instead of 1.0 here is a bug. I don't think you should assert that the bug is present. A future version of influxdb could fix that bug, and you wouldn't want the tests too break then. I think you should just remove this assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The issue you mentioned was closed without a fix, so I doubt this will change soon. It is a result of the default json marshaling in GoLang (they could probably work around it by customizing).
In contrast to you I believe it would be a good thing that the test would fail if the expected result would suddenly change (since the influx versions used to test are pinned it should not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it would be a good thing that the test would fail if the expected result would suddenly change
Yes, but the expected result in this case is1.0, not1, since the database contains a float. The problem is that the library does not return the expected result. I don't think this buggy behavior should be enforced by a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Both are the sameNumber in JSON, which is the reason the issue was closed in InfluxDB.
Idid remove the JSON result from the test - altough I still think it should be there, because it is a way to document the current and future behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, both are the sameNumber in JSON, but that does not make this less of a bug 😛 Python has a different type for int and float, and so does InfluxDB.
And it is indeed a good idea to document the problematic behavior. Maybe you could add something to the doc comments ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I noticed I forgot to add the use_msgpack param docstring to InfluxDBClient doc, so I just pushed a commit documenting that paramter.
Is there a specific place you want me to describe the msgpack/Json difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The doc string of the influxdb client seems like a good place to me.
Also, I'm not sure it's clear: I do not have commit rights on this repository. None of the persons with commit rights are active on the repository anymore.
Uh oh!
There was an error while loading.Please reload this page.
msgpack issue
I identified an issue with the move to the default use ofmsgpack, so added this pull request to revert to the old behaviour, and optionally enable msgpack by setting
use_msgpack=Truein theinit function of the client.The test
test_write_points_mixed_typefails on purpose now because of a difference between the json and msgpack result returned by the server. The question is : is moving to msgpack going to be a breaking change? If so, I propose to merge this pull request (after fixing the test) and let users manually upgrade to msgpack based parsing.This pull request has a couple of other changes identified while getting to the bottom of the above issue.
Fixes
(assertion in mock function): using self.assertRaises() instead.
Others
Contributor checklist