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

Add flag for size based retention #5109

Merged
merged 4 commits into from Jan 18, 2019

Conversation

Projects
None yet
5 participants
@gouthamve
Copy link
Member

gouthamve commented Jan 18, 2019

@krasi-georgiev Could you commit the vendor update, it's taking ages upon ages for me

@krasi-georgiev krasi-georgiev force-pushed the gouthamve:storage-based-retention branch Jan 18, 2019

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Jan 18, 2019

decided to update tsdb in a separate PR to keep the git history separate and reverted the tsdb update commits.

Once the tsdb update pr is merged this one should pass all tests as well.

#5110

gouthamve added some commits Jan 18, 2019

Add flag for size based retention
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Deprecate the old retention flag for a new one.
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

@gouthamve gouthamve force-pushed the gouthamve:storage-based-retention branch to 0179b7b Jan 18, 2019

@codesome
Copy link
Member

codesome left a comment

Nit

Show resolved Hide resolved storage/tsdb/tsdb.go Outdated
Add ability to take a suffix for size flag
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve

This comment has been minimized.

Copy link
Member Author

gouthamve commented Jan 18, 2019

@krasi-georgiev @brian-brazil @SuperQ PTAL.

I am deprecating a flag here and that is worth taking a closer look at.

@simonpasquier
Copy link
Member

simonpasquier left a comment

I'd keep the logic for selecting the correct retention time value in cmd/prometheus/main.go and have storage/tsdb/tsdb.go deal with one parameter only.

@brancz

This comment has been minimized.

Copy link
Member

brancz commented Jan 18, 2019

--storage.tsdb.retention can be deprecated but must continue to work. The new flag can have precedence, but the old one must continue to work until 3.0.

Edit: Sorry it seems that’s what’s happening.

@gouthamve gouthamve force-pushed the gouthamve:storage-based-retention branch Jan 18, 2019

@brancz

This comment has been minimized.

Copy link
Member

brancz commented Jan 18, 2019

Agreed with Simon.

@gouthamve

This comment has been minimized.

Copy link
Member Author

gouthamve commented Jan 18, 2019

Yup, @brancz that is what is happening. And yup, I moved the logic into main.go. It's better, thanks! @simonpasquier

Show resolved Hide resolved cmd/prometheus/main.go Outdated
storage/tsdb/tsdb.go Outdated
@@ -33,6 +33,8 @@ import (
// ErrNotReady is returned if the underlying storage is not ready yet.
var ErrNotReady = errors.New("TSDB not ready")

const DefaultRetentionDuration = "15d"

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Jan 18, 2019

Member

not needed anymore

cmd/prometheus/main.go Outdated
}

promql.LookbackDelta = time.Duration(cfg.lookbackDelta)
promql.SetDefaultEvaluationInterval(time.Duration(config.DefaultGlobalConfig.EvaluationInterval))

logger := promlog.New(&cfg.promlogConfig)

defaultDuration, err := model.ParseDuration(tsdb.DefaultRetentionDuration)

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Jan 18, 2019

Member

As we do the same in chooseRetention(), I'd use a global mode.Duration variable and move this to the init() function.

@gouthamve gouthamve force-pushed the gouthamve:storage-based-retention branch 2 times, most recently Jan 18, 2019

Address feedback
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@brancz

This comment has been minimized.

Copy link
Member

brancz commented Jan 18, 2019

lgtm 👍

@brancz

brancz approved these changes Jan 18, 2019

@gouthamve gouthamve force-pushed the gouthamve:storage-based-retention branch to b875ecb Jan 18, 2019

@gouthamve

This comment has been minimized.

Copy link
Member Author

gouthamve commented Jan 18, 2019

@simonpasquier @brancz Feedback addressed, PTAL.

Thanks for all the rounds of reviews, sorry for the trouble!

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Jan 18, 2019

LGTM

@gouthamve gouthamve merged commit 384cba1 into prometheus:master Jan 18, 2019

2 of 3 checks passed

ci/circleci: build CircleCI is running your tests
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gouthamve gouthamve deleted the gouthamve:storage-based-retention branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.