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.

Sign up for GitHub

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

Jump to bottom

Add function to tsdb.point to get line-protocol string in the correct… #4105

Merged
merged 1 commit into from
Sep 16, 2015
Merged

Add function to tsdb.point to get line-protocol string in the correct… #4105

merged 1 commit into from
Sep 16, 2015

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Sep 15, 2015

@sparrc sparrc mentioned this pull request Sep 15, 2015
Closed
otoolep
otoolep reviewed Sep 15, 2015
View reviewed changes
tsdb/points.go
@@ -1159,6 +1160,9 @@ func (p *point) GetPrecisionMultiplier(precision string) int64 {
return int64(d)
}

// String returns a string representation of the point object, if there is a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: would these comments be better on the interface spec?

@pauldix
Copy link
Member

pauldix commented Sep 15, 2015

I think it would be much better to have SetPrecision convert the given string into a time.Duration. Then when you're working with the timestamp to output it in the string, instead of doing division, use .Round(p.Precision) where precision is a duration.

It's cleaner and I think more readable.

@otoolep
Copy link
Contributor

otoolep commented Sep 15, 2015

Can we see some unit tests? Would that make sense?

@sparrc
Copy link
Contributor Author

sparrc commented Sep 15, 2015

@otoolep updated PR with unit tests and made some of a commenting changes you suggested

@sparrc
Copy link
Contributor Author

sparrc commented Sep 15, 2015

@pauldix I'm not sure this API is working exactly as intended, so I'm going to have to leave in the division as it is right now, I'll send around an email for a separate discussion about what we should do about the write API

@otoolep
Copy link
Contributor

otoolep commented Sep 15, 2015

+1, though I think @pauldix may still have some feedback pre-merge.

@sparrc sparrc merged commit 6d4319d into master Sep 16, 2015
@ekini ekini mentioned this pull request Oct 13, 2015
Closed
@otoolep otoolep deleted the precision-str branch October 28, 2015 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sparrc @pauldix @otoolep @levicook