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.

Sign up for GitHub

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

Jump to bottom

fix retention policy creation inconsistencies #7449

Merged
merged 2 commits into from
Nov 7, 2016
Merged

fix retention policy creation inconsistencies #7449

merged 2 commits into from
Nov 7, 2016

Conversation

corylanou
Copy link
Contributor

@corylanou corylanou commented Oct 11, 2016 •
edited
Loading

This PR fixes inconsistent retention policy logic found in both CreateDatabaseWithRetentionPolicy, CreateRetentionPolicy, and AlterRetentionPolicy.

I have also removed the meta.SetDefaultRetentionPolicy call as it should of never existed to begin with. All defaults should be set with the actual statement calls so they are atomic.

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • Add additional test coverage
  • CHANGELOG.md updated
Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

fixes #7448

corylanou
corylanou commented Oct 11, 2016
View reviewed changes
coordinator/meta_client.go
@@ -12,7 +12,7 @@ type MetaClient interface {
CreateContinuousQuery(database, name, query string) error
CreateDatabase(name string) (*meta.DatabaseInfo, error)
CreateDatabaseWithRetentionPolicy(name string, spec *meta.RetentionPolicySpec) (*meta.DatabaseInfo, error)
CreateRetentionPolicy(database string, spec *meta.RetentionPolicySpec) (*meta.RetentionPolicyInfo, error)
CreateRetentionPolicy(database string, spec *meta.RetentionPolicySpec, makeDefault bool) (*meta.RetentionPolicyInfo, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I'm a fan of the word makeDefault. I'm open to suggestions. setDefault?

@corylanou corylanou force-pushed the cjl-7488-fix-rp-defaults branch from 0317e92 to 5c7bb73 Compare October 11, 2016 18:52
e-dard
e-dard approved these changes Oct 12, 2016
View reviewed changes
Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

Just the one small question, otherwise LGTM 👍

services/meta/data.go
@@ -163,11 +163,21 @@ func (data *Data) CreateRetentionPolicy(database string, rpi *RetentionPolicyInf
if rp.ReplicaN != rpi.ReplicaN || rp.Duration != rpi.Duration || rp.ShardGroupDuration != rpi.ShardGroupDuration {
return ErrRetentionPolicyExists
}
// if they want to make it default, and it's not the default, it's not an identical command so it's an error
if makeDefault && di.DefaultRetentionPolicy != rpi.Name {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may not make any sense, but what if they provide the same RP details as an existing default one, but they don't provide DEFAULT? Should that be allowed, or is that an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With my fix it is now an error. While we don't know why they would do this (although we can assume it was a mistake), we can't alter an RP via a create if it exists. You can only alter WITH an alter. The point of this code is so that if the create is issued with identical specifications, it's idempotent. The intent is to allow them to run the same scripts over and over for create database or retention policy without it erring out when the specifications are identical. If they change their script, then they will have to manually fix any changes to have it run in an idempotent manner again.

@corylanou corylanou force-pushed the cjl-7488-fix-rp-defaults branch 3 times, most recently from e4fcb6b to 40d2ebd Compare October 20, 2016 14:20
@corylanou corylanou force-pushed the cjl-7488-fix-rp-defaults branch from 40d2ebd to 9da8b8d Compare October 21, 2016 18:03
@jwilder jwilder added this to the 1.2.0 milestone Oct 27, 2016
@corylanou corylanou force-pushed the cjl-7488-fix-rp-defaults branch from 9da8b8d to 89b9002 Compare November 3, 2016 14:10
@corylanou corylanou force-pushed the cjl-7488-fix-rp-defaults branch from 89b9002 to 6e29004 Compare November 3, 2016 14:39
@corylanou corylanou merged commit 40f626d into master Nov 7, 2016
@corylanou corylanou deleted the cjl-7488-fix-rp-defaults branch November 7, 2016 14:20
@beckettsean
Copy link
Contributor

@corylanou so now repeated CREATEs with the exact same syntax return a 204? But if there's any difference, they return a 40x?

@corylanou
Copy link
Contributor Author

I would have to check what the server does as far as a status code. But certainly it will no longer work if there is any difference.

@beckettsean
Copy link
Contributor

Please let us know what the actual expected response is, so that we can appropriately document, thanks!

@e-dard e-dard mentioned this pull request Jan 21, 2017
Closed
e-dard added a commit that referenced this pull request Jan 23, 2017
@e-dard
Fixes #7860.
4442cd7 
Fix a regression introduced in #7449.

This commit ensures that create database with retention policy will work
correctly.
@e-dard e-dard mentioned this pull request Jan 23, 2017
Merged
3 tasks
e-dard added a commit that referenced this pull request Jan 23, 2017
@e-dard
Fixes #7860.
63562d9 
Fix a regression introduced in #7449.

This commit ensures that create database with retention policy will work
correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e-dard e-dard e-dard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Retention Policy Inconsistencies
4 participants
@corylanou @beckettsean @e-dard @jwilder