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

change timestamp to time #2405

Merged
merged 1 commit into from
May 11, 2015
Merged

change timestamp to time #2405

merged 1 commit into from
May 11, 2015

Conversation

neonstalwart
Copy link
Contributor

this fixes #2108

i don't know if i went further than was expected. i figured it was easier to do it and get someone to review and provide feedback rather than figure out all the details first.

@neonstalwart
Copy link
Contributor Author

i forgot to mention... i don't know if any of the JavaScript needs updating. i didn't touch it.

@otoolep
Copy link
Contributor

otoolep commented Apr 23, 2015

Thanks @neonstalwart -- at first glance this looks like it. We won't be merging this change immediately, as it requires some co-ordination with some external parters.

@otoolep
Copy link
Contributor

otoolep commented May 8, 2015

@neonstalwart -- we would like to make this change part of the next release. Can you rebase?

@neonstalwart
Copy link
Contributor Author

sure. if I don't get to it over the weekend then I should be able to get that in on Monday

@neonstalwart
Copy link
Contributor Author

@otoolep... i've rebased and Test_ServerSingleGraphiteIntegration_NoDatabase works for me locally. could you kick the CI again to see what happens?

Value float64 `json:"value"`
Tags map[string]string `json:"tags,omitempty"`
Metric string `json:"metric"`
Time int64 `json:"time"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not familiar with opentsdb. did i do the right thing here or is the json going to be "timestamp" because it's not something we control?

@neonstalwart
Copy link
Contributor Author

also, should i change shardGroupByTimestamp to be shardGroupByTime or just leave it as it is?

@otoolep
Copy link
Contributor

otoolep commented May 11, 2015

Thanks @neonstalwart -- I'll kick the CI system now. We're still tracking down the races in the code which result in a failure every so often.

To be the change as simple as possible, I suggest not renaming any functions just yet.

@neonstalwart
Copy link
Contributor Author

i've changed opentsdb so that the internal tsdbDP has a Time field which is the json "timestamp" field. http://opentsdb.net/docs/build/html/api_http/put.html#example-single-data-point-put seems to suggest that this is what we should expect from opentsdb json but i thought that consistently using Time in the go data structures was a good idea.

hopefully the build passes again 😥

@neonstalwart
Copy link
Contributor Author

@otoolep could you kick CI again please? i had a real failure that i've addressed but now this is back to random failures.

@otoolep
Copy link
Contributor

otoolep commented May 11, 2015

Green build.

Thanks @neonstalwart

@toddboom -- merge at will. +1 from me.

toddboom added a commit that referenced this pull request May 11, 2015
@toddboom toddboom merged commit 730e8cd into influxdata:master May 11, 2015
@neonstalwart neonstalwart deleted the time branch May 11, 2015 19:38
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.

Rename timestamp to time for consistency
3 participants