Make default retention policy configurable

[sklevenz]

For example:

Introduce two variables in config.toml file

"DefaultRetentionPolicy": "7d",
"DefaultShardDuration": "1d",

to make default retention configurable.

[yujinqiu]

+1

[mnuessler]

+1

[talbright]

I'm taking a crack at this, getting pretty close...

[jeanpralo]

👍

[otoolep]

The reason we've held off on this is that what if the user subsequently alters the default retention policy? Should the system always override on restart? We can make it so that the config settings only apply when the retention policy is created.

[jeanpralo]

Just my thought but I would rather have the default retention policy applied at the creation of a database, and if someone wants to alter it they can do it manually.

[otoolep]

This would definitely be a useful feature. The steps to implement this are as follows:

• add support for a new config variable to this section: https://github.com/influxdb/influxdb/blob/master/etc/config.sample.toml#L31 called retetention-auto-duration. The code changes would be here: https://github.com/influxdb/influxdb/blob/master/meta/config.go
• when a retention policy is auto-created in CreateDatabase in the meta package, the rpi.Duration should be set to any configured value, not the default of AutoCreateRetentionPolicyPeriod

[pires]

+1 for applying default policy only on database creation. On it!

[pires]

@otoolep what would be the desired defaults?

[pires]

Actually, @otoolep I was looking at #2805 and read the following by @pauldix:

Stuff in the toml config file should be host specific only. Retention policies are actually set at a cluster level. They're meta data that exist in the cluster metastore (regardless if you're running only a single server), so it doesn't really make sense to have this in the toml file.

Does it still applies? If so, this issue should be closed. Otherwise, #2805 should be reviewed once more because it seems to deliver. I would just add a couple tests where we guarantee a couple things:

• defaults are enforced on database creation
• values configured by the user are properly enforced on database creation

[otoolep]

I can check with @pauldix but I think this is actually OK. We've done some similar things recently, and the key thing to understand is that the retention policy duration only applies on creation. If you look at the code, the duration is set to something, and right now it's set to infinity (0).

This change would be easy enough -- one simply defines the key, and if it's not set, one goes what the code does now. The PR you referenced is pretty close, except the default time is wrong.

I need to check with @pauldix to see if I can get him to agree if this change is actually OK. In practise I can see why users like the default retention policy, but don't like that it's always of infinite retention.

[pires]

Sure. Keep us posted.

[pauldix]

What's the desired result of this setting? That every time you create a database it has a given retention duration?

I think the more appropriate thing to do is to add something to the CREATE DATABASE command that lets you specify what the default duration would be. I think that would be super useful.

[pires]

@pauldix something like CREATE DATABASE WITH RETENTION=24h NOAA_water_database?

[otoolep]

I was wondering if should really be the full command, including name etc. But that could be over the top.

CREATE DATABASE NOAA_water_database WITH RETENTION 24h

might be more in line with our current standards -- the optional keywords come after the mandatory keywords, and we don't use = in the commands.

[pires]

@otoolep can you please lead the way spec'ing this and I'll give it a try?

[otoolep]

Sure.

The first step would be understanding how to add a command to our parser. It's not that difficult actually, if you can follow an example. For example this PR shows how the DROP DATABASE command was modified to accept optional keywords:

#4698

Note that CREATE DATABASE currently takes optional IF NOT EXISTS keywords, so you might like to check out that code too. I guess the full command is something like:

CREATE DATABASE NOAA_water_database [ [ WITH RETENTION 24hr] [IF NOT EXISTS] ]

[otoolep]

Once the parsing and AST code has been completed, it's pretty easy to work with the CreateDatabase type, and just check for the presence of the require attibutes, and when creating the database, create the retention policy (today that is checked via a flag in the meta package, which is what @pauldix doesn't like).

[pires]

@otoolep thank you so much! Seems really doable even for a (Go and InfluxDB) newbie like me.

Some questions though:

• The syntax seems to be CREATE DATABASE [IF NOT EXISTS] <database_name>. Or is the parser smart enough to accept options after the name?
• Should we follow time.ParseDuration(string) spec and use 24h instead of 24hr. Probably your example has a typo but want to make sure before jumping in.

[otoolep]

@pires -- actually if you checked the code you are right, I got it backwards. The parser is not smart, and will error if you supply what I thought it was.

As for the time spec, I was just being brief. Yes, it should follow the Go spec as you suggest.

[pires]

@otoolep owning it. Should I report back to this issue?

[otoolep]

So I am guessing we want:

CREATE DATABASE [IF NOT EXISTS] NOAA_water_database [ WITH RETENTION 24hr ]

To be honest, once you know how to add commands to the parser it's not a big deal to modify if we change our mind during development.

[otoolep]

That sounds good, thanks @pires

[pires]

To be honest, once you know how to add commands to the parser it's not a big deal to modify if we change our mind during development.

Yes, your pointers made whoever takes this task life easier. Again, I'm going for it. Thank you!