Use BatchPoints for writing from client library

[corylanou]

Several issues had been logged recently, and most of it was due to the client library taking on too much responsibility. This PR will get rid of a custom type used for writing, and use the BatchPoints that are used throughout our system.

This will fix #1849 in the process.

• Adding omitempty to both Point and BatchPoint to cut down on marshaling and http overhead
• No longer ignore error when Marshaling to json
• Removed custom Timestamp type
• Write signature now takes BatchPoint instead of custom type
• Moved BatchPoint back to client package
• Added a lot more test coverage.

[otoolep]

Makes sense so far, let me know when it needs final review.

[vladlopes]

@corylanou; Fully aggre with the new approach. I do think that, as the server is already referencing client structs, the use of BatchPoint is the way to go. Also thanks for using the omitempty mentioned in my PR.

Altough a working in progress, I have noted that the current code is still:

• Using Unmarshal instead of Marshal
• Discarding possible marshalling error
• Treating the response body even when the status code is http.StatusOK, when the body will be empty and an error will be generated (EOF)

[vladlopes]

Also noted that:

• The omitempty was included in Point instead of BatchPoints. This way, the client is still marshalling all fields;
• The Timestamp type is still in the code

[vladlopes]

@corylanou Be aware that the omitempty in BatchPoints for the Timestamp field will have to be treated as the Point one. Also I think that mandatory fields (Database and Points in BatchPoints and Fields in Point) should not have the omitempty, just as a hint in the code that those fields should always be provided.

[corylanou]

@vladlopes I went back and forth on the omitempty. If it is to be used for performance reasons, then they should all be allowed to be empty. No checks happen until it gets to the server, in which case, missing data points are then err'd and sent back. Since the go compiler doesn't enforce this, anyone can send an empty BatchPoints, and there is no reason to marshal anything if it's empty. At that point, let the server intercept it and send back an appropriate message.

As for the "hint" it might provide, I would prefer to have documentation (not written yet) on how to use the client library in which we describe what is optional/not optional. I will also put a comment at the top of the BatchPoints and Point struct explaining the omitempty.

Let me know if that is in line with your concerns and if that satisfies them. I'm very open to suggestions on this.

Thanks!

[vladlopes]

@corylanou I totally agree with you. On second thought, as the compiler doesn't enforce this, the documentation solution is really the way to go.

As you provided custom marshalling for Point Timestamp field, which is the heavy part of the message, and it won't be marshalled if it is Zero, I think it would be a much less concern to always send the BatchPoints Timestamp field.

Have you thought about the server response when everything is good (status code == http.StatusOK) ?

Sorry if I am bodering you with this little things, thanks for the work and please let me know if I can do anything else to help!

[corylanou]

Ready for review.

[otoolep]

All these tests need to move away from the types, and just do string compares -- using types in the integration tests is too brittle, and too difficult to read.

Can you check out the table-driven tests in the other parts of this file to see what I mean?

[otoolep]

And if len is 0, what should happen? Isn't that unexpected?

[corylanou]

For this endpoint, if it's successful (meaning we wrote the data), we just respond with a status code 200, there is no body to decode.

[corylanou]

Should also add that if there IS an error, that is when we write out a body, and then you need to decode it.

[evanphx]

Is the response data always small? If there is any chance it could be large, don't use ReadAll, instead stream decode like was done before.

[corylanou]

@evanphx yeah, that's a good point. I looked at it twice when I did my code reviews and felt it kind of smelled. Really I should just check if the error is EOF, as that is the normal case. If it is, move on. If not, throw an error.

[otoolep]

Change generally makes sense, but the integration tests need to be string-compare based. They are too difficult to follow in their current form, and too difficult to maintain.

[corylanou]

Fixes #1844, #1849, #1850, #1662, #1680, #1683, #1691