Modify the default retention policy name and make it configurable #6586

jsternberg · 2016-05-09T20:18:17Z

The default retention policy name is changed to "autogen" instead of
"default" since it ends up being ambiguous when we tell a user to check
the default retention policy, it is uncertain if we are referring to the
default retention policy (which can be changed) or the retention policy
with the name "default".

Now the automatically generated retention policy name is "autogen".

The default retention policy is now also configurable through the
configuration file so an administrator can customize what they think
should be the default.

Fixes #3733.

jsternberg · 2016-05-09T20:24:08Z

@beckettsean @jwilder

This should be a good starting point. A lingering concern of mine with this is that some of the meta commands are not atomic and may work badly in clustering if an outage happens. It's possible to get into a state when creating a database with a default retention policy for the database and retention policy to be created, but the default isn't set. In that scenario, the default will never end up getting set without using a different command.

In all likelyhood, the data store should probably support CreateDatabaseWithRetentionPolicy as a single function rather than issuing 3 meta commands to implement that. Another way to solve that issue is to make CreateDatabaseWithRetentionPolicy also set the default retention policy if it hasn't previously been set. Right now, you can do the following commands and get a possibly weird result:

CREATE DATABASE mydb WITH NAME "default" # default retention policy is "default"
CREATE DATABASE mydb WITH NAME "autogen" # default retention policy is still "default"

I would think the second command should either return an error or set the default retention policy if it differs from "NAME".

beckettsean · 2016-05-09T21:54:43Z

@jsternberg I don't quite follow the cluster issue. Wouldn't any CREATE DATABASE have to go through consensus and either fail or succeed atomically for the cluster?

jsternberg · 2016-05-09T23:13:34Z

@beckettsean yes, but if it's three separate commands then 2 of the 3 could go through. It appears as though in clustering we have this as a single command rather than 3 though so I don't see it as an issue in the open source version.

We still have the issue I mentioned in the open source version. If I run the two CREATE DATABASE commands from above, what should be the end result of the system?

beckettsean · 2016-05-09T23:35:52Z

@jsternberg shouldn't the second command fail because mydb already exists?

jsternberg · 2016-05-09T23:40:23Z

Right now the second command won't fail. Database creates don't fail unless there is something different in the command. Retention policy creates would be the same so, as long as the existing retention policy has the same parameters, it would not fail in its creation.

The interesting part is that the second will work and create the retention policy, but it won't update the default retention policy even though it looks like it would. That's why I was asking if we should make it into an error if you try to use CREATE DATABASE WITH NAME "retention_policy" and the retention policy name is different than the current default retention policy or if we should have that command update the default retention policy.

beckettsean · 2016-05-11T23:22:46Z

@jsternberg thanks for the clarifications. I am not yet conversant with the new CREATE DATABASE syntax, it seems.

I would propose that a second CREATE DATABASE statement with a conflicting retention policy name should throw an error. Not creating a database has immediate and obvious feedback and won't lead to anything difficult to undo. Inadvertently changing the default retention policy means data is suddenly going to the wrong place, and fixing it is painful.

e-dard · 2016-05-13T19:44:38Z

services/meta/config.go

-		RetentionAutoCreate: true,
-		LeaseDuration:       toml.Duration(DefaultLeaseDuration),
-		LoggingEnabled:      DefaultLoggingEnabled,
+		RetentionAutoCreate:        true,


The idiom with Influx configs is to add DefaultDefaultRetentionPolicyName = "autogen" as a constant (even though the name is silly), and then set DefaultRetentionPolicyName: DefaultDefaultRetentionPolicyName in the constructor.

That way other packages can figure out the default without having to construct a config.

Is DefaultRetentionPolicyName fine? The double Default seems a bit silly, but I'll do it if there's an objection to removing the extra default.

It's fine, I just like consistency is all 😆

e-dard · 2016-05-13T19:49:14Z

Aside from adding the const for the default name, LGTM 👍

I think it should have an extra pair of eyes on it given the changes to the meta client methods.

jsternberg · 2016-05-13T19:54:21Z

Updating the PR with that change.

The default retention policy name is changed to "autogen" instead of "default" since it ends up being ambiguous when we tell a user to check the default retention policy, it is uncertain if we are referring to the default retention policy (which can be changed) or the retention policy with the name "default". Now the automatically generated retention policy name is "autogen". The default retention policy is now also configurable through the configuration file so an administrator can customize what they think should be the default. Fixes #3733.

jwilder · 2016-05-25T15:48:27Z

@corylanou @dgnorton Can you take a look?

dgnorton · 2016-06-06T19:51:46Z

services/meta/client.go

 			ReplicaN: 1,
 		}); err != nil {
 			return nil, err
 		}
-		if err := data.SetDefaultRetentionPolicy(name, "default"); err != nil {
+		if err := data.SetDefaultRetentionPolicy(name, ""); err != nil {


When database creation is complete, does this mean there is no default RP?

No, this will set the default retention policy to be whatever the default retention policy name should be (from the config).

Ah, I see it in the implementation of the method now.

dgnorton · 2016-06-06T20:03:17Z

👍

sparrc · 2016-06-08T11:43:19Z

This needs to be communicated better. This will break every existing telegraf user.

It also means that Telegraf 1.0.0-beta1 is not compatible with InfluxDB 1.0.0-beta1 😵

Since 1.0, InfluxDB has changed the name of the default retention policy from 'default' to 'autogen' to reduce naming confusion. This should be reflected in the default settings for this project. influxdata/influxdb#6586

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from b837112 to b27f7c2 Compare May 9, 2016 20:27

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from b27f7c2 to 7c83fff Compare May 9, 2016 23:21

jsternberg force-pushed the js-3733-rename-default-retention-policy branch 2 times, most recently from a2028d3 to cb15307 Compare May 10, 2016 18:15

jsternberg added this to the 1.0.0 milestone May 10, 2016

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from cb15307 to 49f5487 Compare May 12, 2016 16:12

e-dard reviewed May 13, 2016
View reviewed changes

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from 49f5487 to c1a54d0 Compare May 13, 2016 19:52

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from c1a54d0 to 85038f0 Compare May 14, 2016 02:35

jsternberg mentioned this pull request May 16, 2016

Support specifying a retention policy for the graphite service #6640

Merged

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from 85038f0 to c3e785b Compare May 16, 2016 20:21

jwilder assigned corylanou May 19, 2016

jsternberg force-pushed the js-3733-rename-default-retention-policy branch 3 times, most recently from a023bda to 83330fc Compare May 23, 2016 21:06

jsternberg force-pushed the js-3733-rename-default-retention-policy branch from 83330fc to baaa782 Compare May 24, 2016 13:51

dgnorton reviewed Jun 6, 2016
View reviewed changes

jsternberg merged commit b8e22d9 into master Jun 6, 2016

jsternberg deleted the js-3733-rename-default-retention-policy branch June 6, 2016 20:05

rkuchan mentioned this pull request Jun 6, 2016

The name of the default RP is different/configurable influxdata/docs.influxdata.com-ARCHIVE#495

Closed

wyc mentioned this pull request Jun 14, 2017

Update Default Rentention Policy Name jenkinsci/influxdb-plugin#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the default retention policy name and make it configurable #6586

Modify the default retention policy name and make it configurable #6586

jsternberg commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 11, 2016

e-dard May 13, 2016

jsternberg May 13, 2016

e-dard May 13, 2016

e-dard commented May 13, 2016

jsternberg commented May 13, 2016

jwilder commented May 25, 2016

dgnorton Jun 6, 2016

jsternberg Jun 6, 2016

dgnorton Jun 6, 2016

dgnorton commented Jun 6, 2016

sparrc commented Jun 8, 2016

Modify the default retention policy name and make it configurable #6586

Modify the default retention policy name and make it configurable #6586

Conversation

jsternberg commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 9, 2016

jsternberg commented May 9, 2016

beckettsean commented May 11, 2016

e-dard May 13, 2016

Choose a reason for hiding this comment

jsternberg May 13, 2016

Choose a reason for hiding this comment

e-dard May 13, 2016

Choose a reason for hiding this comment

e-dard commented May 13, 2016

jsternberg commented May 13, 2016

jwilder commented May 25, 2016

dgnorton Jun 6, 2016

Choose a reason for hiding this comment

jsternberg Jun 6, 2016

Choose a reason for hiding this comment

dgnorton Jun 6, 2016

Choose a reason for hiding this comment

dgnorton commented Jun 6, 2016

sparrc commented Jun 8, 2016