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

Remove influxdb client precision specification, and unused JSON funcs #4099

Closed
wants to merge 1 commit into from

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Sep 15, 2015

/cc @otoolep @corylanou @jwilder

This became a bit of a rabbit-hole, but basically this is eliminating two things: Setting precision at the influxDB client level, and marshalling and unmarshalling client BatchPoint objects.

There was a bug encountered in Telegraf where if you were writing a set of BatchPoints that had their Time field manually set, and then specified a Precision other than ns, InfluxDB would think you were writing very far into the future (see telegraf issue influxdata/telegraf#175)

The JSON marshalling part was just a side-effect because there was a lot of code around specifying precision. I spoke with @otoolep about this and we decided it may just be better to delete that code since it's deprecated anyways.

@jwilder
Copy link
Contributor

jwilder commented Sep 15, 2015

This looks like a breaking change. Anyone that was depending on precision fields or something other than nanoseconds will either fail to compile or get unexpected results next time they update their client. Should update the changelog too.

I'm in favor of removing this code since it adds unnecessary complexity. Not sure what @corylanou @pauldix think though?

@sparrc
Copy link
Contributor Author

sparrc commented Sep 15, 2015

@jwilder I'm actually not sure this is the right solution. Users can write points without specifying a timestamp (but specifying precision), and the DB will truncate time.Now() to the given precision.

So to match that behavior, maybe the InfluxDB client should just truncate the given timestamp to the given precision.

@otoolep
Copy link
Contributor

otoolep commented Sep 15, 2015

I did not know that without a timestamp the system will truncate to the
given precision.

But actually truncating a given timestamp to the specified precision, with
the possible loss of precision, that seems like the wrong thing to do.

Cameron and I discussed this in depth yesterday. Perhaps there is an
application of that "feature" I would be missing.

On Tuesday, September 15, 2015, Cameron Sparr notifications@github.com
wrote:

@jwilder https://github.com/jwilder I'm actually not sure this is the
right solution. Users can write points without specifying a timestamp (but
specifying precision), and the DB will truncate time.Now() to the given
precision.

So to match that behavior, maybe the InfluxDB client should just truncate
the given timestamp to the given precision.


Reply to this email directly or view it on GitHub
#4099 (comment).

@sparrc
Copy link
Contributor Author

sparrc commented Sep 15, 2015

It's tricky because if we remove this and say that users of the client need to truncate their own timestamps, then we would also be removing the ability to write data without timestamps at a specific precision

@otoolep
Copy link
Contributor

otoolep commented Sep 15, 2015

I did not know about that application of "precison" when we discussed the
field.

Hmmm.

On Tuesday, September 15, 2015, Cameron Sparr notifications@github.com
wrote:

It's tricky because if we remove this and say that users of the client
need to truncate their own timestamps, then we would also be removing the
ability to write data without timestamps at a specific precision


Reply to this email directly or view it on GitHub
#4099 (comment).

@sparrc
Copy link
Contributor Author

sparrc commented Sep 15, 2015

closing in favor of #4105

@sparrc sparrc closed this Sep 15, 2015
@pauldix
Copy link
Member

pauldix commented Sep 15, 2015

Truncating to the specified precision is exactly what the database should do. Some people expect that if they write a point at the same second, it will overwrite the previous one, and to ensure that happens truncating at second precision is necessary.

The truncation will become even more important with the new storage engine as it will be able to compress timestamps much better when they are lower precision.

@otoolep
Copy link
Contributor

otoolep commented Sep 15, 2015

OK @pauldix -- I did not realise that is what we wanted.

@sparrc sparrc deleted the rm-client-precision branch May 21, 2016 21:01
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.

5 participants