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

Improves existing RP checking when creating a DB. #6202

Merged
merged 2 commits into from
Apr 13, 2016

Conversation

chris-ramon
Copy link
Contributor

Details

  • Currently when running an InfluxQL(CREATE DATABASE IF NOT EXISTS xyz WITH DURATION 24h0m0s REPLICATION 1 NAME one_day_only) to create a database which does not provide shard duration a default value will be set for it via:

  • When re-running the same InfluxQL mentioned above, an error will occur (retention policy conflicts with an existing policy) when it shouldn't, based on the expected behavior of IF NOT EXISTS & confirmed by the docs:

    // Check if the retention policy already exists. If it does and matches
    // the desired retention policy, exit with no error.
    • So current strategy for checking retention policies does not consider the default shard duration value mentioned above.
  • TL:DR, this PR improves the checking for existing retention policy within meta.Client.CreateDatabaseWithRetentionPolicy(...) to consider the default shard duration value which is set when none is provided when creating a database.


  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@e-dard
Copy link
Contributor

e-dard commented Apr 13, 2016

@chris-ramon thanks for this, looks good. However you'll need to move your CHANGELOG entry to under bugfixes for 0.13. I suggest you rebase from master to get a more recent CHANGELOG.

@e-dard e-dard added this to the 0.13.0 milestone Apr 13, 2016
@chris-ramon
Copy link
Contributor Author

@chris-ramon thanks for this, looks good. However you'll need to move your CHANGELOG entry to under bugfixes for 0.13. I suggest you rebase from master to get a more recent CHANGELOG.

Hi @fred-influx, thanks for reviewing it! - I've addressed it on 93aa287.

@e-dard
Copy link
Contributor

e-dard commented Apr 13, 2016

LGTM 👍

@e-dard e-dard merged commit f029d80 into influxdata:master Apr 13, 2016
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.

None yet

2 participants