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

Cherry-pick TimeSeriesDatabase from megacarbon branch #484

Closed
wants to merge 7 commits into from
Closed

Cherry-pick TimeSeriesDatabase from megacarbon branch #484

wants to merge 7 commits into from

Conversation

iain-buclaw-sociomantic
Copy link
Contributor

It is becoming quite a pet peeve of mine to be stuck on the abandoned megacarbon branch and having to manually merge in upstream fixes. :-)

While the megacarbon branch does a lot more, it is really only this part that is really relevant (to me), where all database actions are abstracted away, allowing any other database implementation to be plugged in.

I've stopped short of duplicating parseRetentionDefs from whisper and adding a ceres backend. If there is no object against such things, I will push forward with those too. However what's been changed here is enough to allow a pause for review.

@mleinart
Copy link
Member

mleinart commented Dec 3, 2015

Thanks, I'll try and take a look at this today
On Dec 3, 2015 4:18 AM, "Iain Buclaw" [email protected] wrote:

It is becoming quite a pet peeve of mine to be stuck on the abandoned
megacarbon branch and having to manually merge in upstream fixes. :-)

While the megacarbon branch does a lot more, it is really only this part
that is really relevant (to me), where all database actions are abstracted
away, allowing any other database implementation to be plugged in.

I've stopped short of duplicating parseRetentionDefs from whisper and
adding a ceres backend. If there is no object against such things, I will
push forward with those too. However what's been changed here is enough to

allow a pause for review.

You can view, comment on, or merge this pull request online at:

#484
Commit Summary

  • Add abstract TimeSeriesDatabase class with Whisper override
  • Move whisper configuration to WhisperDatabase
  • Move write action into WhisperDatabase
  • Move exists action into WhisperDatabase
  • Move create action into WhisperDatabase
  • Move get/set metadata action into WhisperDatabase
  • Remove dependency on getFilesystemPath

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#484.

@iain-buclaw-sociomantic
Copy link
Contributor Author

OK, thanks.

FYI, I've finished the conversion here: master...iain-buclaw-sociomantic:tsdatabase2

Doing some small tests and it all works fine with both whisper and ceres.

@iain-buclaw-sociomantic
Copy link
Contributor Author

Rebased, and fiddled with the getFilesystemPath test until it passed.

@mleinart
Copy link
Member

Note: when I say "I'll look at this today" it often means "I'll start feeling guilty about not looking at this starting today" ;)

I promise I haven't forgotten about this. This is something that has needed to be done for some time, thanks for the work!

@mleinart
Copy link
Member

I can kindof go either way with the change to TimeSeriesDatabase.create()'s method signature from (self, metric, **options) to specifying the options explicitly, but I'm just going to ship as is.

FYI, I'd like to hold off merging Ceres from the other branch until graphite-web supports reading past retentions which is something I've been working on getting put in

mleinart added a commit that referenced this pull request Dec 18, 2015
Cherry-pick TimeSeriesDatabase from megacarbon branch
@mleinart
Copy link
Member

Merged in eeba611

@mleinart mleinart closed this Dec 18, 2015
@iain-buclaw-sociomantic iain-buclaw-sociomantic deleted the tsdatabase branch December 20, 2015 14:05
@iain-buclaw-sociomantic
Copy link
Contributor Author

Note: when I say "I'll look at this today" it often means "I'll start feeling guilty about not looking at this starting today" ;)

I am on holidays so there is no rush on my side, yet. :-)

I can kindof go either way with the change to TimeSeriesDatabase.create()'s method signature from (self, metric, **options) to specifying the options explicitly, but I'm just going to ship as is.

In megacarbon, there is a determine_metadata function which returns a value to pass as this param. I'm not really fond of the signature, but when considering any of the following.

  • The absence of any function/method to return 'determined' metadata that is relevant for the storage database.
  • Whisper and Ceres use the same config options.
  • I'm not aware of any other storage backend for carbon-cache (other tsdb's implement the carbon protocol natively).

I chose the quick path for now, with intent to properly address it later.

FYI, I'd like to hold off merging Ceres from the other branch until graphite-web supports reading past retentions which is something I've been working on getting put in

This is something I implemented in Ceres itself for now (I think you know this), I should rebase that PR for FYI purposes. Good to know that someone is working on pushing it out to the client though, that is definitely the right way to go about it.

I'll raise a supplementary PR then which completes the TimeSeriesDatabase API (but not the create signature), stopping short of actual Ceres support.

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.

2 participants