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 delete time fields creating unparseable points #8251

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Conversation

jwilder
Copy link
Contributor

@jwilder jwilder commented Apr 4, 2017

If a field was named time was written and was subsequently dropped,
it could leave a trailing comma in the series key causing it to fail
to be parseable in other parts of the code.

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@jwilder jwilder requested a review from joelegasse April 4, 2017 21:17
@jwilder jwilder added the review label Apr 4, 2017
@jwilder jwilder requested a review from dgnorton April 4, 2017 21:58
models/points.go Outdated
@@ -1919,7 +1919,13 @@ func (p *point) Delete() {
switch {
case p.it.end == p.it.start:
case p.it.end >= len(p.fields):
p.fields = p.fields[:p.it.start]
// If we're not deleting first field, trim the comma
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be if we're not deleting the last field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to use bytes.TrimSuffix

If a field was named time was written and was subsequently dropped,
it could leave a trailing comma in the series key causing it to fail
to be parseable in other parts of the code.
@jwilder jwilder merged commit abaf42f into master Apr 4, 2017
@jwilder jwilder deleted the jw-time-delete branch April 4, 2017 22:37
@jwilder jwilder removed the review label Apr 4, 2017
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.

3 participants