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

Fix client.Write function #1849

Closed
wants to merge 1 commit into from

Conversation

vladlopes
Copy link
Contributor

Fix #1844

Besides those issues raised by georgmu, I have found that:

  • The current client couldn't send the same tags, timestamp and precision for all points, as the server is able to receive (client.Write != influxdb.BatchPoints);
  • There was an error being ignored when enconding the data to json;
  • When the write was successfull the status returned was 200 and the body was empty, which generated an error when decoding the results (EOF).

@d2g
Copy link

d2g commented Mar 5, 2015

Duplicate of a combination of:
#1662 #1680 #1683 #1691

@vladlopes
Copy link
Contributor Author

@d2g - None of these other pull requests change the way data is written to the server. The server is not expecting an array of writes, just one write with multiple points. They also don't treat the server response correctly.

@d2g
Copy link

d2g commented Mar 5, 2015

@vladlopes in #1662 Cory's comment @d2g states that it should support both a single and an array. I'm sure when one of the influxdb team gets some focus on this area they'll see the issues and provide some direction so we can contribute in moving the client forward.

@corylanou
Copy link
Contributor

Sorry I haven't been responsive to this issue. I've been on vacation this week. I get back in the office tomorrow and I'll get on this first thing. At first glance, this looks good. I want to add some more testing to show how this endpoint should be used for a single write, or by keeping the http connection open and streaming multiple batch writes without closing it down. I think that should give better clarity to how this endpoint can be used, as well as keep test coverage going.

It's also fair to note that this endpoint was written prior to us supporting precision, so it looks like when we added that feature, this did not get updated as a result (my bad). I'll try to make sure if we make changes to the data structure for writing data in the future this endpoint is updated as part of that request.

Thanks for all your hard work and hopefully we'll get this merged in a day or two once I write more integration tests around it.

@oliveagle
Copy link
Contributor

👍 have the same issue. waiting to merge this PR

@vladlopes
Copy link
Contributor Author

I would like also to give some more ideas to improve the client/server writing (I don't know if we should open a new issue for this):

  • Maybe get rid of the client.Timestamp type. As of today, I don't think it adds anything to time.Time as the default json serialization already uses RFC3339Nano;
  • Include omitempty attribute for optional values (retention policy, tags, timestamp and precision), reducing the payload and documenting also wich attributes are optional directly in the code;
  • If we remove the Timestamp type and uses time.Time directly, we can omitempty it when this issue (encoding/json, encoding/xml: self-defined zero types golang/go#4357) get solved - there is already a commit ready to be merged. Today, the only way of omitempty the timestamp field would be to make it a pointer (*time.Time), wich I think we should do as this is increases considerable the payload from client to server, specially for clients that want to use the server current time;

@corylanou
Copy link
Contributor

It would appear that the client.Timestamp has outlived its purpose, so I agree that should go away now.

Seems fair that the omitempty could be added.

Agree that moving this to a separate PR is appropriate, as it allows this immediate issue to get resolved, followed up by the next one.

@corylanou
Copy link
Contributor

Closing this issue. It is being addressed in pr #1868 with a little bit different approach. Feel free to comment on that PR with any questions/suggestions.

@corylanou corylanou closed this Mar 6, 2015
@vladlopes vladlopes deleted the fix-client-write branch March 9, 2015 17:59
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

Successfully merging this pull request may close these issues.

client.Write() is broken
4 participants